From 9453e1d9551e56bc7c8d73c19a29e609de873f15 Mon Sep 17 00:00:00 2001 From: Sebastian Spaeth Date: Thu, 19 Jan 2012 11:24:16 +0100 Subject: [PATCH] Fix getuidvalidity crash (UIDVALIDITY returning None) Rename getuidvalidity -> get_uidvalidity and cache the IMAP result. 1) Start modernizing our function names using more underscores 2) IMAPs implementation of get_uidvalidity was removing the UIDVALIDITY result from the imaplib2 result stack, so subsequent calls would return "None". As various functions can invoke this, this led to some errors that we avoid by caching the current UIDVALIDITY value in the Folder instance. There are more simplifications and improvements to be made. Signed-off-by: Sebastian Spaeth --- Changelog.draft.rst | 1 + offlineimap/accounts.py | 2 +- offlineimap/folder/Base.py | 17 +++++++++++------ offlineimap/folder/IMAP.py | 22 ++++++++++++++-------- offlineimap/folder/Maildir.py | 6 ++++-- offlineimap/ui/Machine.py | 2 +- offlineimap/ui/UIBase.py | 4 ++-- 7 files changed, 34 insertions(+), 20 deletions(-) diff --git a/Changelog.draft.rst b/Changelog.draft.rst index 83cab97..93f64f1 100644 --- a/Changelog.draft.rst +++ b/Changelog.draft.rst @@ -26,3 +26,4 @@ Bug Fixes * Fix python2.6 compatibility with the TTYUI backend (crash) * Fix TTYUI regression from 6.5.2 in refresh loop (crash) +* Fix crashes related to UIDVALIDITY returning "None" diff --git a/offlineimap/accounts.py b/offlineimap/accounts.py index fc2687f..ad2015a 100644 --- a/offlineimap/accounts.py +++ b/offlineimap/accounts.py @@ -378,7 +378,7 @@ def syncfolder(account, remotefolder, quick): statusfolder = statusrepos.getfolder(remotefolder.getvisiblename().\ replace(remoterepos.getsep(), statusrepos.getsep())) - if localfolder.getuidvalidity() == None: + if localfolder.get_uidvalidity() == None: # This is a new folder, so delete the status cache to be sure # we don't have a conflict. statusfolder.deletemessagelist() diff --git a/offlineimap/folder/Base.py b/offlineimap/folder/Base.py index 9ff60e2..cab22c2 100644 --- a/offlineimap/folder/Base.py +++ b/offlineimap/folder/Base.py @@ -110,13 +110,14 @@ class BaseFolder(object): return basename def isuidvalidityok(self): - """Does the cached UID match the real UID + """Tests if the cached UIDVALIDITY match the real current one - If required it caches the UID. In this case the function is not - threadsafe. So don't attempt to call it from concurrent threads.""" + If required it saves the UIDVALIDITY value. In this case the + function is not threadsafe. So don't attempt to call it from + concurrent threads.""" if self.getsaveduidvalidity() != None: - return self.getsaveduidvalidity() == self.getuidvalidity() + return self.getsaveduidvalidity() == self.get_uidvalidity() else: self.saveuidvalidity() return 1 @@ -142,7 +143,7 @@ class BaseFolder(object): This function is not threadsafe, so don't attempt to call it from concurrent threads.""" - newval = self.getuidvalidity() + newval = self.get_uidvalidity() uidfilename = self._getuidfilename() file = open(uidfilename + ".tmp", "wt") @@ -151,7 +152,11 @@ class BaseFolder(object): os.rename(uidfilename + ".tmp", uidfilename) self._base_saved_uidvalidity = newval - def getuidvalidity(self): + def get_uidvalidity(self): + """Retrieve the current connections UIDVALIDITY value + + This function needs to be implemented by each Backend + :returns: UIDVALIDITY as a (long) number""" raise NotImplementedException def cachemessagelist(self): diff --git a/offlineimap/folder/IMAP.py b/offlineimap/folder/IMAP.py index 75c731c..ffbd6b1 100644 --- a/offlineimap/folder/IMAP.py +++ b/offlineimap/folder/IMAP.py @@ -65,17 +65,23 @@ class IMAPFolder(BaseFolder): def getcopyinstancelimit(self): return 'MSGCOPY_' + self.repository.getname() - def getuidvalidity(self): + def get_uidvalidity(self): + """Retrieve the current connections UIDVALIDITY value + + UIDVALIDITY value will be cached on the first call. + :returns: The UIDVALIDITY as (long) number.""" + if hasattr(self, '_uidvalidity'): + # use cached value if existing + return self._uidvalidity imapobj = self.imapserver.acquireconnection() try: - # SELECT receives UIDVALIDITY response + # SELECT (if not already done) and get current UIDVALIDITY self.selectro(imapobj) - # note: we would want to use .response() here but that - # often seems to return [None], even though we have - # data. TODO - uidval = imapobj._get_untagged_response('UIDVALIDITY') - assert uidval != [None], "response('UIDVALIDITY') returned [None]!" - return long(uidval[-1]) + typ, uidval = imapobj.response('UIDVALIDITY') + assert uidval != [None] and uidval != None, \ + "response('UIDVALIDITY') returned [None]!" + self._uidvalidity = long(uidval[-1]) + return self._uidvalidity finally: self.imapserver.releaseconnection(imapobj) diff --git a/offlineimap/folder/Maildir.py b/offlineimap/folder/Maildir.py index 95258d9..fe85308 100644 --- a/offlineimap/folder/Maildir.py +++ b/offlineimap/folder/Maildir.py @@ -83,8 +83,10 @@ class MaildirFolder(BaseFolder): """Return the absolute file path to the Maildir folder (sans cur|new)""" return self._fullname - def getuidvalidity(self): - """Maildirs have no notion of uidvalidity, so we just return a magic + def get_uidvalidity(self): + """Retrieve the current connections UIDVALIDITY value + + Maildirs have no notion of uidvalidity, so we just return a magic token.""" return 42 diff --git a/offlineimap/ui/Machine.py b/offlineimap/ui/Machine.py index ab57ea2..271629c 100644 --- a/offlineimap/ui/Machine.py +++ b/offlineimap/ui/Machine.py @@ -70,7 +70,7 @@ class MachineUI(UIBase): def validityproblem(s, folder): s._printData('validityproblem', "%s\n%s\n%s\n%s" % \ (folder.getname(), folder.getrepository().getname(), - folder.getsaveduidvalidity(), folder.getuidvalidity())) + folder.getsaveduidvalidity(), folder.get_uidvalidity())) def connecting(s, hostname, port): s._printData('connecting', "%s\n%s" % (hostname, str(port))) diff --git a/offlineimap/ui/UIBase.py b/offlineimap/ui/UIBase.py index 66ecd4f..1cdd94b 100644 --- a/offlineimap/ui/UIBase.py +++ b/offlineimap/ui/UIBase.py @@ -309,9 +309,9 @@ class UIBase(object): def validityproblem(self, folder): self.logger.warning("UID validity problem for folder %s (repo %s) " "(saved %d; got %d); skipping it. Please see FAQ " - "and manual how to handle this." % \ + "and manual on how to handle this." % \ (folder, folder.getrepository(), - folder.getsaveduidvalidity(), folder.getuidvalidity())) + folder.getsaveduidvalidity(), folder.get_uidvalidity())) def loadmessagelist(self, repos, folder): self.logger.debug("Loading message list for %s[%s]" % (