From 06bfff7c04ce74892368a7bda229ad397aa5b921 Mon Sep 17 00:00:00 2001 From: Sebastian Spaeth Date: Thu, 18 Aug 2011 09:08:53 +0200 Subject: [PATCH 1/8] Coalesce SEARCH uid list into sequence set Rather than passing in huge lists of continuous numbers which eventually overflow the maximum command line length, we coalesce number ranges before passing the UID sequence to SEARCH. This should do away with the error that has been reported with busy mailing lists and 'maxage'. Signed-off-by: Sebastian Spaeth Signed-off-by: Nicolas Sebrecht --- offlineimap/folder/IMAP.py | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/offlineimap/folder/IMAP.py b/offlineimap/folder/IMAP.py index 20940a2..f930bc5 100644 --- a/offlineimap/folder/IMAP.py +++ b/offlineimap/folder/IMAP.py @@ -143,14 +143,13 @@ class IMAPFolder(BaseFolder): search_condition += "SMALLER " + self.config.getdefault("Account " + self.accountname, "maxsize", -1) search_condition += ")" - searchresult = imapobj.search(None, search_condition) - #result would come back seperated by space - to change into a fetch - #statement we need to change space to comma - messagesToFetch = searchresult[1][0].replace(" ", ",") + res_type, res_data = imapobj.search(None, search_condition) + #result UIDs come back seperated by space + messagesToFetch = imaputil.uid_sequence(res_data.split()) except KeyError: return - if len(messagesToFetch) < 1: + if not messagesToFetch: # No messages; return return else: @@ -172,7 +171,7 @@ class IMAPFolder(BaseFolder): # Now, get the flags and UIDs for these. # We could conceivably get rid of maxmsgid and just say # '1:*' here. - response = imapobj.fetch(messagesToFetch, '(FLAGS UID)')[1] + response = imapobj.fetch("'%s'" % messagesToFetch, '(FLAGS UID)')[1] finally: self.imapserver.releaseconnection(imapobj) for messagestr in response: From 8532a69458f5256ba73be2d46dc5200dd81dcb17 Mon Sep 17 00:00:00 2001 From: Sebastian Spaeth Date: Thu, 18 Aug 2011 09:08:54 +0200 Subject: [PATCH 2/8] Clean up the maxsize maxage code Do away with the wrapping of this code in a try...except KeyError, as this code cannot conceivably throw a KeyError. Even if it could, it should be documented why we should simply return() in this case. Shorten some of the variable names and minor code cleanup while taking the git blame anyway. Signed-off-by: Sebastian Spaeth Signed-off-by: Nicolas Sebrecht --- offlineimap/folder/IMAP.py | 57 ++++++++++++++++++-------------------- 1 file changed, 27 insertions(+), 30 deletions(-) diff --git a/offlineimap/folder/IMAP.py b/offlineimap/folder/IMAP.py index f930bc5..fa2c377 100644 --- a/offlineimap/folder/IMAP.py +++ b/offlineimap/folder/IMAP.py @@ -107,51 +107,48 @@ class IMAPFolder(BaseFolder): # TODO: Make this so that it can define a date that would be the oldest messages etc. def cachemessagelist(self): - imapobj = self.imapserver.acquireconnection() + maxage = self.config.getdefaultint("Account %s" % self.accountname, + "maxage", -1) + maxsize = self.config.getdefaultint("Account %s" % self.accountname, + "maxsize", -1) self.messagelist = {} + imapobj = self.imapserver.acquireconnection() + try: # Primes untagged_responses imaptype, imapdata = imapobj.select(self.getfullname(), readonly = 1, force = 1) - maxage = self.config.getdefaultint("Account " + self.accountname, "maxage", -1) - maxsize = self.config.getdefaultint("Account " + self.accountname, "maxsize", -1) - if (maxage != -1) | (maxsize != -1): - try: - search_condition = "("; + search_cond = "("; - if(maxage != -1): - #find out what the oldest message is that we should look at - oldest_time_struct = time.gmtime(time.time() - (60*60*24*maxage)) + if(maxage != -1): + #find out what the oldest message is that we should look at + oldest_struct = time.gmtime(time.time() - (60*60*24*maxage)) - #format this manually - otherwise locales could cause problems - monthnames_standard = ["Jan", "Feb", "Mar", "Apr", "May", \ - "Jun", "Jul", "Aug", "Sep", "Oct", "Nov", "Dec"] + #format months manually - otherwise locales cause problems + monthnames = ["Jan", "Feb", "Mar", "Apr", "May", \ + "Jun", "Jul", "Aug", "Sep", "Oct", "Nov", "Dec"] - our_monthname = monthnames_standard[oldest_time_struct[1]-1] - daystr = "%(day)02d" % {'day' : oldest_time_struct[2]} - date_search_str = "SINCE " + daystr + "-" + our_monthname \ - + "-" + str(oldest_time_struct[0]) + month = monthnames[oldest_struct[1]-1] + daystr = "%(day)02d" % {'day' : oldest_struct[2]} - search_condition += date_search_str + search_cond += "SINCE %s-%s-%s" % (daystr, month, + oldest_struct[0]) - if(maxsize != -1): - if(maxage != -1): #There are two conditions - add a space - search_condition += " " + if(maxsize != -1): + if(maxage != -1): # There are two conditions, add space + search_cond += " " + search_cond += "SMALLER %d" % maxsize - search_condition += "SMALLER " + self.config.getdefault("Account " + self.accountname, "maxsize", -1) + search_cond += ")" - search_condition += ")" - - res_type, res_data = imapobj.search(None, search_condition) - #result UIDs come back seperated by space - messagesToFetch = imaputil.uid_sequence(res_data.split()) - except KeyError: - return + res_type, res_data = imapobj.search(None, search_cond) + # Result UIDs seperated by space, coalesce into ranges + messagesToFetch = imaputil.uid_sequence(res_data.split()) if not messagesToFetch: - # No messages; return - return + return # No messages to sync + else: # 1. Some mail servers do not return an EXISTS response # if the folder is empty. 2. ZIMBRA servers can return From f755c8b4231c9ac01eae0d534dac56d4f96ae475 Mon Sep 17 00:00:00 2001 From: Sebastian Spaeth Date: Thu, 18 Aug 2011 09:08:55 +0200 Subject: [PATCH 3/8] Use range 1:* if we want to examine all messages in a folder Some code cleanup. If we want to examine all messages of a folder, don't try to find out how many there are and request a long list of all of them, but simply request 1:*. This obliviates us from the need to force a select even if we already had the folder selected and it requires us to send a few less bytes over the wire. Signed-off-by: Sebastian Spaeth Signed-off-by: Nicolas Sebrecht --- offlineimap/folder/IMAP.py | 33 +++++++++------------------------ 1 file changed, 9 insertions(+), 24 deletions(-) diff --git a/offlineimap/folder/IMAP.py b/offlineimap/folder/IMAP.py index fa2c377..8c42419 100644 --- a/offlineimap/folder/IMAP.py +++ b/offlineimap/folder/IMAP.py @@ -105,7 +105,6 @@ class IMAPFolder(BaseFolder): self.imapserver.releaseconnection(imapobj) return False - # TODO: Make this so that it can define a date that would be the oldest messages etc. def cachemessagelist(self): maxage = self.config.getdefaultint("Account %s" % self.accountname, "maxage", -1) @@ -114,10 +113,11 @@ class IMAPFolder(BaseFolder): self.messagelist = {} imapobj = self.imapserver.acquireconnection() - try: - # Primes untagged_responses - imaptype, imapdata = imapobj.select(self.getfullname(), readonly = 1, force = 1) + res_type, imapdata = imapobj.select(self.getfullname(), True) + + # By default examine all UIDs in this folder + msgsToFetch = '1:*' if (maxage != -1) | (maxsize != -1): search_cond = "("; @@ -149,28 +149,13 @@ class IMAPFolder(BaseFolder): if not messagesToFetch: return # No messages to sync - else: - # 1. Some mail servers do not return an EXISTS response - # if the folder is empty. 2. ZIMBRA servers can return - # multiple EXISTS replies in the form 500, 1000, 1500, - # 1623 so check for potentially multiple replies. - if imapdata == [None]: - return - - maxmsgid = 0 - for msgid in imapdata: - maxmsgid = max(long(msgid), maxmsgid) - if maxmsgid < 1: - #no messages; return - return - messagesToFetch = '1:%d' % maxmsgid; - - # Now, get the flags and UIDs for these. - # We could conceivably get rid of maxmsgid and just say - # '1:*' here. - response = imapobj.fetch("'%s'" % messagesToFetch, '(FLAGS UID)')[1] + # Get the flags and UIDs for these. single-quotes prevent + # imaplib2 from quoting the sequence. + res_type, response = imapobj.fetch("'%s'" % msgsToFetch, + '(FLAGS UID)') finally: self.imapserver.releaseconnection(imapobj) + for messagestr in response: # Discard the message number. messagestr = messagestr.split(' ', 1)[1] From 33029403821a1e862786e559bb04cfb9b97f59d4 Mon Sep 17 00:00:00 2001 From: Sebastian Spaeth Date: Thu, 18 Aug 2011 09:08:56 +0200 Subject: [PATCH 4/8] Proper error handling for SEARCH and FETCH failures from the server SEARCH and FETCH were never checking that the IMAP server actually returned OK. Throw OfflineImapErrors at severity FOLDER in case one of them fails. Signed-off-by: Sebastian Spaeth Signed-off-by: Nicolas Sebrecht --- offlineimap/folder/IMAP.py | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/offlineimap/folder/IMAP.py b/offlineimap/folder/IMAP.py index 8c42419..417b80a 100644 --- a/offlineimap/folder/IMAP.py +++ b/offlineimap/folder/IMAP.py @@ -144,15 +144,28 @@ class IMAPFolder(BaseFolder): search_cond += ")" res_type, res_data = imapobj.search(None, search_cond) - # Result UIDs seperated by space, coalesce into ranges - messagesToFetch = imaputil.uid_sequence(res_data.split()) - if not messagesToFetch: + if res_type != 'OK': + raise OfflineImapError("SEARCH in folder [%s]%s failed. " + "Search string was '%s'. Server responded '[%s] %s'" % ( + self.getrepository(), self, + search_cond, res_type, res_data), + OfflineImapError.ERROR.FOLDER) + + # Result UIDs are seperated by space, coalesce into ranges + msgsToFetch = imaputil.uid_sequence(res_data.split()) + if not msgsToFetch: return # No messages to sync # Get the flags and UIDs for these. single-quotes prevent # imaplib2 from quoting the sequence. res_type, response = imapobj.fetch("'%s'" % msgsToFetch, '(FLAGS UID)') + if res_type != 'OK': + raise OfflineImapError("FETCHING UIDs in folder [%s]%s failed. " + "Server responded '[%s] %s'" % ( + self.getrepository(), self, + res_type, response), + OfflineImapError.ERROR.FOLDER) finally: self.imapserver.releaseconnection(imapobj) From fb8017991c3de0fd3111f47338e4648e1abb586d Mon Sep 17 00:00:00 2001 From: Sebastian Spaeth Date: Mon, 22 Aug 2011 12:21:11 +0200 Subject: [PATCH 5/8] Rework undocumented listjoin to create UID sequences This function was badly named and completely undocumented. Rework it to avoid copying the full UID list using an iterator. Make it possible to hand it a list of UIDs as strings rather than implicitely relying on the fact that they are numeric already. Document the code. The behavior off the function itself remained otherwise unchanged. Signed-off-by: Sebastian Spaeth Signed-off-by: Nicolas Sebrecht --- offlineimap/folder/Gmail.py | 2 +- offlineimap/folder/IMAP.py | 2 +- offlineimap/imaputil.py | 44 ++++++++++++++++++------------------- 3 files changed, 23 insertions(+), 25 deletions(-) diff --git a/offlineimap/folder/Gmail.py b/offlineimap/folder/Gmail.py index 7cbff4f..587a00f 100644 --- a/offlineimap/folder/Gmail.py +++ b/offlineimap/folder/Gmail.py @@ -55,7 +55,7 @@ class GmailFolder(IMAPFolder): try: imapobj.select(self.getfullname()) result = imapobj.uid('copy', - imaputil.listjoin(uidlist), + imaputil.uid_sequence(uidlist), self.trash_folder) assert result[0] == 'OK', \ "Bad IMAPlib result: %s" % result[0] diff --git a/offlineimap/folder/IMAP.py b/offlineimap/folder/IMAP.py index 417b80a..80f144d 100644 --- a/offlineimap/folder/IMAP.py +++ b/offlineimap/folder/IMAP.py @@ -626,7 +626,7 @@ class IMAPFolder(BaseFolder): self.ui.flagstoreadonly(self, uidlist, flags) return r = imapobj.uid('store', - imaputil.listjoin(uidlist), + imaputil.uid_sequence(uidlist), operation + 'FLAGS', imaputil.flagsmaildir2imap(flags)) assert r[0] == 'OK', 'Error with store: ' + '. '.join(r[1]) diff --git a/offlineimap/imaputil.py b/offlineimap/imaputil.py index 3905851..b336876 100644 --- a/offlineimap/imaputil.py +++ b/offlineimap/imaputil.py @@ -168,35 +168,33 @@ def flagsmaildir2imap(maildirflaglist): retval.sort() return '(' + ' '.join(retval) + ')' -def listjoin(list): - start = None - end = None - retval = [] +def uid_sequence(uidlist): + """Collapse UID lists into shorter sequence sets - def getlist(start, end): + [1,2,3,4,5,10,12,13] will return "1:5,10,12:13". This function does + not sort the list, and only collapses if subsequent entries form a + range. + :returns: The collapsed UID list as string""" + def getrange(start, end): if start == end: return(str(start)) - else: - return(str(start) + ":" + str(end)) - + return "%s:%s" % (start, end) - for item in list: - if start == None: - # First item. - start = item - end = item - elif item == end + 1: - # An addition to the list. - end = item - else: - # Here on: starting a new list. - retval.append(getlist(start, end)) - start = item - end = item + if not len(uidlist): return '' # Empty list, return + start, end = None, None + retval = [] - if start != None: - retval.append(getlist(start, end)) + for item in iter(uidlist): + item = int(item) + if start == None: # First item + start, end = item, item + elif item == end + 1: # Next item in a range + end = item + else: # Starting a new range + retval.append(getrange(start, end)) + start, end = item, item + retval.append(getrange(start, end)) # Add final range/item return ",".join(retval) From 1917be8e83da4c4a5abb83874405a2795c176b60 Mon Sep 17 00:00:00 2001 From: Sebastian Spaeth Date: Mon, 22 Aug 2011 12:21:12 +0200 Subject: [PATCH 6/8] Shorten list of messages to be deleted in UI output Rather than output the full list of messages, coalesce it into number ranges wherever possible. Signed-off-by: Sebastian Spaeth Signed-off-by: Nicolas Sebrecht --- offlineimap/ui/UIBase.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/offlineimap/ui/UIBase.py b/offlineimap/ui/UIBase.py index 16e10bc..f5dca57 100644 --- a/offlineimap/ui/UIBase.py +++ b/offlineimap/ui/UIBase.py @@ -302,7 +302,7 @@ class UIBase: ds = s.folderlist(destlist) s._msg("Deleting %d messages (%s) in %s" % \ (len(uidlist), - ", ".join([str(u) for u in uidlist]), + offlineimap.imaputil.uid_sequence(uidlist), ds)) def addingflags(s, uidlist, flags, dest): From 971ed3adacc037e3e1899419a18fa2aeedf79e01 Mon Sep 17 00:00:00 2001 From: Sebastian Spaeth Date: Tue, 6 Sep 2011 13:19:25 +0200 Subject: [PATCH 7/8] Allow Imapserver.releaseconnection() to drop a connection If a connection is broken, we want to have it really dropped and not be reused. So far, we are checking the .Terminate attribute for this, but according to the imaplib2 author, it is only set on normal shutdown and it is an undocumented attribute whose meaning could change any time. This patch introduces the parameter drop_conn which allows to tell releaseconnection() that we really want to connection being dropped from the pool of available connections and properly destroy it. Signed-off-by: Sebastian Spaeth Signed-off-by: Nicolas Sebrecht --- offlineimap/imapserver.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/offlineimap/imapserver.py b/offlineimap/imapserver.py index 707464d..40bbb8d 100644 --- a/offlineimap/imapserver.py +++ b/offlineimap/imapserver.py @@ -109,12 +109,15 @@ class IMAPServer: return self.root - def releaseconnection(self, connection): - """Releases a connection, returning it to the pool.""" + def releaseconnection(self, connection, drop_conn=False): + """Releases a connection, returning it to the pool. + + :param drop_conn: If True, the connection will be released and + not be reused. This can be used to indicate broken connections.""" self.connectionlock.acquire() self.assignedconnections.remove(connection) # Don't reuse broken connections - if connection.Terminate: + if connection.Terminate or drop_conn: connection.logout() else: self.availableconnections.append(connection) From 4c558c1b69bf16d399631c6be28ceb9d386c28f9 Mon Sep 17 00:00:00 2001 From: Sebastian Spaeth Date: Tue, 6 Sep 2011 13:19:26 +0200 Subject: [PATCH 8/8] Error proof IMAP.APPEND against dropped connections Make sure that when a connection is dropped during append, we really discard the broken connection and get a new one, retrying. We retry indefinitely on the specific abort() Exception, as this is what imaplib2 suggests us to do. Signed-off-by: Sebastian Spaeth Signed-off-by: Nicolas Sebrecht --- offlineimap/folder/IMAP.py | 88 +++++++++++++++++++++----------------- 1 file changed, 49 insertions(+), 39 deletions(-) diff --git a/offlineimap/folder/IMAP.py b/offlineimap/folder/IMAP.py index 80f144d..8049eb1 100644 --- a/offlineimap/folder/IMAP.py +++ b/offlineimap/folder/IMAP.py @@ -21,6 +21,7 @@ import random import binascii import re import time +from sys import exc_info from copy import copy from Base import BaseFolder from offlineimap import imaputil, imaplibutil, OfflineImapError @@ -483,53 +484,62 @@ class IMAPFolder(BaseFolder): self.savemessageflags(uid, flags) return uid + imapobj = self.imapserver.acquireconnection() try: - imapobj = self.imapserver.acquireconnection() + success = False # succeeded in APPENDING? + while not success: - try: - imapobj.select(self.getfullname()) # Needed for search and making the box READ-WRITE - except imapobj.readonly: - # readonly exception. Return original uid to notify that - # we did not save the message. (see savemessage in Base.py) - self.ui.msgtoreadonly(self, uid, content, flags) - return uid + # UIDPLUS extension provides us with an APPENDUID response. + use_uidplus = 'UIDPLUS' in imapobj.capabilities - # UIDPLUS extension provides us with an APPENDUID response to our append() - use_uidplus = 'UIDPLUS' in imapobj.capabilities + # get the date of the message, so we can pass it to the server. + date = self.getmessageinternaldate(content, rtime) + content = re.sub("(?200: + dbg_output = "%s...%s" % (content[:150], content[-50:]) + else: + dbg_output = content + self.ui.debug('imap', "savemessage: date: %s, content: '%s'" % + (date, dbg_output)) - if not use_uidplus: - # insert a random unique header that we can fetch later - (headername, headervalue) = self.generate_randomheader(content) - self.ui.debug('imap', 'savemessage: new header is: %s: %s' % \ - (headername, headervalue)) - content = self.savemessage_addheader(content, headername, - headervalue) - if len(content)>200: - dbg_output = "%s...%s" % (content[:150], - content[-50:]) - else: - dbg_output = content - self.ui.debug('imap', "savemessage: date: %s, content: '%s'" % - (date, dbg_output)) + try: + # Select folder for append and make the box READ-WRITE + imapobj.select(self.getfullname()) + except imapobj.readonly: + # readonly exception. Return original uid to notify that + # we did not save the message. (see savemessage in Base.py) + self.ui.msgtoreadonly(self, uid, content, flags) + return uid - #Do the APPEND - try: - (typ, dat) = imapobj.append(self.getfullname(), + #Do the APPEND + try: + (typ, dat) = imapobj.append(self.getfullname(), imaputil.flagsmaildir2imap(flags), date, content) - except Exception, e: - # If the server responds with 'BAD', append() raise()s directly. - # So we need to prepare a response ourselves. - typ, dat = 'BAD', str(e) - if typ != 'OK': #APPEND failed - raise OfflineImapError("Saving msg in folder '%s', repository " - "'%s' failed. Server reponded; %s %s\nMessage content was:" - " %s" % (self, self.getrepository(), typ, dat, dbg_output), - OfflineImapError.ERROR.MESSAGE) + success = True + except imapobj.abort, e: + # connection has been reset, release connection and retry. + self.ui.error(e, exc_info()[2]) + self.imapserver.releaseconnection(imapobj, True) + imapobj = self.imapserver.acquireconnection() + except imapobj.error, e: + # If the server responds with 'BAD', append() raise()s directly. + # So we need to prepare a response ourselves. + typ, dat = 'BAD', str(e) + if typ != 'OK': #APPEND failed + raise OfflineImapError("Saving msg in folder '%s', repository " + "'%s' failed. Server reponded; %s %s\nMessage content was:" + " %s" % (self, self.getrepository(), typ, dat, dbg_output), + OfflineImapError.ERROR.MESSAGE) # Checkpoint. Let it write out stuff, etc. Eg searches for # just uploaded messages won't work if we don't do this. (typ,dat) = imapobj.check()