[PATCH] fix hang because of infinite loop reading EOF
Haojun Bao
baohaojun at gmail.com
Thu Mar 3 09:48:26 GMT 2011
Sebastian Spaeth <Sebastian at SSpaeth.de> writes:
> On Thu, 03 Mar 2011 10:46:33 +0800, Haojun Bao <baohaojun at gmail.com> wrote:
>> Offlineimap sometimes hangs when downloading gmail with large
>> attachment.
>>
>> This is because of several infinite loop when reading the socket while
>> the EOF has happened. It can be seen from 2 facts:
>
> Hi,
>
> I think your patch might be safe and should most probably be included
> (with some comment in the code explaining why we need that check and
> abort I guess).
Yeah, I think so too. It's the right thing to do. It should be the
semantics of the read method. If you read the read(2) system call
manpage, it tells you that when EOF is seen, return value is 0; it does
not say ``loop forever when EOF happen''.
And in python's case, an empty string should be returned when EOF happen.
> However, will that be enough? If we are in the state you describe the
> connection has basically been closed and the file descriptor is
> worthless, right?
Yes, that will be enough, I think. Again, think about the read(2) system
call.
>
> So it is ok to abort the read(), but it will not close the connection,
> and we will be trying to read from the same connection soon again. And
> subsequent read() attempts will also not be successful then, right?
>
Yes, exactly. It's all very similar to read() and close() system calls.
> So what we should do in a case where we detect a broken connection is to
> properly kill off the connection and re-establish it.
No, at least not in the read() method.
An alternative I prefer is to just abort the program. Which is alreay in
current version, after the EOF detection is patched. You can see the
following exception:
WARNING: ERROR attempting to copy message 344 for account Gmail:Traceback (most recent call last):
File "/usr/lib/pymodules/python2.6/offlineimap/folder/Base.py", line 282, in copymessageto
message = self.getmessage(uid)
File "/usr/lib/pymodules/python2.6/offlineimap/folder/IMAP.py", line 216, in getmessage
initialresult = imapobj.uid('fetch', '%d' % uid, '(BODY.PEEK[])')
File "/usr/lib/python2.6/imaplib.py", line 753, in uid
typ, dat = self._simple_command(name, command, *args)
File "/usr/lib/python2.6/imaplib.py", line 1060, in _simple_command
return self._command_complete(name, self._command(name, *args))
File "/usr/lib/python2.6/imaplib.py", line 890, in _command_complete
raise self.abort('command: %s => %s' % (name, val))
abort: command: UID => socket error: EOF
>
> Or am I completely wrong here?
>
>> After reading the mailing list, I see Gábor Melis has proposed a patch
>> (SO_KEEPALIVE) to fix hang, I don't know if my case is same as his. But
>> anyway, I think the several read methods contain bugs in their
>> implementation.
>
> THe SO_KEEPALIVE will certainly help to prevent some connection closes,
> but that is basically not related to the fact that python's read() on
> SSL objects doesn't seem to fail when the connection has been closed
> already,
Oh, I think you are confused here. The socket is in CLOSE_WAIT, not
closed (we have not called close() yet, on the other end, gmail called
it:-).
So python SSL is doing the right thing, returning empty string to
indicate EOF. And we should follow it in our wrapped version.
> so I think you might be right about some bugs looming in the
> ssl read() functionality. Again, I am not sure that just aborting the
> read() loop will be enough to fix the hangs, as we won't close the
> connection and try to use the same ssl socket to read soon enough
> again. At least that is what I would suspect without having checked.
>
Don't worry. Somebody will notice the EOF and exception will be thrown
finally. See above.
>> You can use the following patch to fix the problem.
>> (I have posted it on the news server, but not sure did it get through.)
>
> I am all for including these, if there is a nice commit message
> or code comment, explaining why this is basically needed.
OK. Please see my updated commit message below:
Read() should return empty string when EOF happen, instead of looping
forever. This is the right semantics of read(), and a wrapped version
should not change it.
Signed-off-by: Bao Haojun <baohaojun at gmail.com>
---
offlineimap/imaplibutil.py | 9 ++++++++-
offlineimap/imapserver.py | 4 ++++
2 files changed, 12 insertions(+), 1 deletions(-)
diff --git a/offlineimap/imaplibutil.py b/offlineimap/imaplibutil.py
index 69d3983..ad98ab8 100644
--- a/offlineimap/imaplibutil.py
+++ b/offlineimap/imaplibutil.py
@@ -47,7 +47,10 @@ class IMAP4_Tunnel(IMAP4):
def read(self, size):
retval = ''
while len(retval) < size:
- retval += self.infd.read(size - len(retval))
+ buf = self.infd.read(size - len(retval))
+ if not buf:
+ break
+ retval += buf
return retval
def readline(self):
@@ -200,6 +203,8 @@ class WrappedIMAP4_SSL(IMAP4_SSL):
read = 0
while read < n:
data = self._read_upto (n-read)
+ if not data:
+ break
read += len(data)
chunks.append(data)
@@ -212,6 +217,8 @@ class WrappedIMAP4_SSL(IMAP4_SSL):
retval = ''
while 1:
linebuf = self._read_upto(1024)
+ if not linebuf:
+ return retval
nlindex = linebuf.find("\n")
if nlindex != -1:
retval += linebuf[:nlindex + 1]
diff --git a/offlineimap/imapserver.py b/offlineimap/imapserver.py
index 2a9f247..c298aa3 100644
--- a/offlineimap/imapserver.py
+++ b/offlineimap/imapserver.py
@@ -71,6 +71,8 @@ class UsefulIMAP4(UsefulIMAPMixIn, imaplibutil.WrappedIMAP4):
io = StringIO()
while read < size:
data = imaplib.IMAP4.read (self, min(size-read,8192))
+ if not data:
+ break
read += len(data)
io.write(data)
return io.getvalue()
@@ -86,6 +88,8 @@ class UsefulIMAP4_SSL(UsefulIMAPMixIn, imaplibutil.WrappedIMAP4_SSL):
io = StringIO()
while read < size:
data = imaplibutil.WrappedIMAP4_SSL.read (self, min(size-read,8192))
+ if not data:
+ break
read += len(data)
io.write(data)
return io.getvalue()
--
1.7.2.3
More information about the OfflineIMAP-project
mailing list