From e7fabf9e57b2c10e1262d167814f6ba0985bfe40 Mon Sep 17 00:00:00 2001 From: Eygene Ryabinkin Date: Sun, 11 Jan 2015 23:44:24 +0300 Subject: [PATCH 1/5] Properly re-raise exception to save original tracebacks We usually mutate some exceptions to OfflineImapError() and it is a whole lot better if such exception will show up with the original traceback, so all valid occurrences of such mutations were transformed to the 3-tuple form of "raise". Had also added coding guidelines document where this re-raise strategy is documented. Signed-off-by: Eygene Ryabinkin --- docs/CodingGuidelines.rst | 102 ++++++++++++++++++++++++++++++++ docs/doc-src/CodingGuidelines.rst | 1 + offlineimap/CustomConfig.py | 3 +- offlineimap/accounts.py | 3 +- offlineimap/folder/Gmail.py | 4 +- offlineimap/folder/GmailMaildir.py | 4 +- offlineimap/folder/IMAP.py | 9 ++- offlineimap/folder/LocalStatus.py | 5 +- offlineimap/folder/LocalStatusSQLite.py | 3 +- offlineimap/folder/Maildir.py | 6 +- offlineimap/folder/UIDMaps.py | 6 +- offlineimap/imaplibutil.py | 3 +- offlineimap/imapserver.py | 16 ++--- offlineimap/repository/IMAP.py | 3 +- offlineimap/repository/__init__.py | 8 ++- 15 files changed, 150 insertions(+), 26 deletions(-) create mode 100644 docs/CodingGuidelines.rst create mode 120000 docs/doc-src/CodingGuidelines.rst diff --git a/docs/CodingGuidelines.rst b/docs/CodingGuidelines.rst new file mode 100644 index 0000000..d6d29a2 --- /dev/null +++ b/docs/CodingGuidelines.rst @@ -0,0 +1,102 @@ +.. -*- coding: utf-8 -*- +.. _OfflineIMAP: https://github.com/OfflineIMAP/offlineimap +.. _OLI_git_repo: git://github.com/OfflineIMAP/offlineimap.git + +================================= +Coding guidelines for OfflineIMAP +================================= + +.. contents:: +.. .. sectnum:: + +This document contains assorted guidelines for programmers that want +to hack OfflineIMAP. + + +------------------ +Exception handling +------------------ + +OfflineIMAP on many occasions re-raises various exceptions and often +changes exception type to `OfflineImapError`. This is not a problem +per se, but you must always remember that we need to preserve original +tracebacks. This is not hard if you follow these simple rules. + +For re-raising original exceptions, just use:: + + raise + +from inside your exception handling code. + +If you need to change exception type, or its argument, or whatever, +use this three-argument form:: + + raise YourExceptionClass(argum, ents), None, sys.exc_info()[2] + +In this form, you're creating an instance of new exception, so ``raise`` +will deduce its ``type`` and ``value`` parameters from the first argument, +thus the second expression passed to ``raise`` is always ``None``. +And the third one is the traceback object obtained from the thread-safe +``exc_info()`` function. + +In fact, if you hadn't already imported the whole ``sys`` module, it will +be better to import just ``exc_info()``:: + + from sys import exc_info + +and raise like this:: + + raise YourExceptionClass(argum, ents), None, exc_info()[2] + +since this is the historically-preferred style in the OfflineIMAP code. +.. -*- coding: utf-8 -*- +.. _OfflineIMAP: https://github.com/OfflineIMAP/offlineimap +.. _OLI_git_repo: git://github.com/OfflineIMAP/offlineimap.git + +================================= +Coding guidelines for OfflineIMAP +================================= + +.. contents:: +.. .. sectnum:: + +This document contains assorted guidelines for programmers that want +to hack OfflineIMAP. + + +------------------ +Exception handling +------------------ + +OfflineIMAP on many occasions re-raises various exceptions and often +changes exception type to `OfflineImapError`. This is not a problem +per se, but you must always remember that we need to preserve original +tracebacks. This is not hard if you follow these simple rules. + +For re-raising original exceptions, just use:: + + raise + +from inside your exception handling code. + +If you need to change exception type, or its argument, or whatever, +use this three-argument form:: + + raise YourExceptionClass(argum, ents), None, sys.exc_info()[2] + +In this form, you're creating an instance of new exception, so ``raise`` +will deduce its ``type`` and ``value`` parameters from the first argument, +thus the second expression passed to ``raise`` is always ``None``. +And the third one is the traceback object obtained from the thread-safe +``exc_info()`` function. + +In fact, if you hadn't already imported the whole ``sys`` module, it will +be better to import just ``exc_info()``:: + + from sys import exc_info + +and raise like this:: + + raise YourExceptionClass(argum, ents), None, exc_info()[2] + +since this is the historically-preferred style in the OfflineIMAP code. diff --git a/docs/doc-src/CodingGuidelines.rst b/docs/doc-src/CodingGuidelines.rst new file mode 120000 index 0000000..7117956 --- /dev/null +++ b/docs/doc-src/CodingGuidelines.rst @@ -0,0 +1 @@ +../CodingGuidelines.rst \ No newline at end of file diff --git a/offlineimap/CustomConfig.py b/offlineimap/CustomConfig.py index 8b070c5..44cfcab 100644 --- a/offlineimap/CustomConfig.py +++ b/offlineimap/CustomConfig.py @@ -16,6 +16,7 @@ import os import re +from sys import exc_info try: from ConfigParser import SafeConfigParser, Error @@ -75,7 +76,7 @@ class CustomConfigParser(SafeConfigParser): return re.split(separator_re, val) except re.error as e: raise Error("Bad split regexp '%s': %s" % \ - (separator_re, e)) + (separator_re, e)), None, exc_info()[2] def getdefaultlist(self, section, option, default, separator_re): """Same as getlist, but returns the value of `default` diff --git a/offlineimap/accounts.py b/offlineimap/accounts.py index d9c9b88..91314f9 100644 --- a/offlineimap/accounts.py +++ b/offlineimap/accounts.py @@ -210,7 +210,8 @@ class SyncableAccount(Account): self._lockfd.close() raise OfflineImapError("Could not lock account %s. Is another " "instance using this account?" % self, - OfflineImapError.ERROR.REPO) + OfflineImapError.ERROR.REPO), \ + None, exc_info()[2] def _unlock(self): """Unlock the account, deleting the lock file""" diff --git a/offlineimap/folder/Gmail.py b/offlineimap/folder/Gmail.py index 69f0075..c12c0ff 100644 --- a/offlineimap/folder/Gmail.py +++ b/offlineimap/folder/Gmail.py @@ -17,6 +17,7 @@ # Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA import re +from sys import exc_info from offlineimap import imaputil, OfflineImapError from offlineimap import imaplibutil @@ -144,7 +145,8 @@ class GmailFolder(IMAPFolder): raise OfflineImapError("FETCHING UIDs in folder [%s]%s failed. " % \ (self.getrepository(), self) + \ "Server responded '[%s] %s'" % \ - (res_type, response), OfflineImapError.ERROR.FOLDER) + (res_type, response), OfflineImapError.ERROR.FOLDER), \ + None, exc_info()[2] finally: self.imapserver.releaseconnection(imapobj) diff --git a/offlineimap/folder/GmailMaildir.py b/offlineimap/folder/GmailMaildir.py index 3b127d2..5ca0e1f 100644 --- a/offlineimap/folder/GmailMaildir.py +++ b/offlineimap/folder/GmailMaildir.py @@ -17,6 +17,7 @@ import os +from sys import exc_info from .Maildir import MaildirFolder from offlineimap import OfflineImapError import offlineimap.accounts @@ -170,7 +171,8 @@ class GmailMaildirFolder(MaildirFolder): os.rename(tmppath, filepath) except OSError as e: raise OfflineImapError("Can't rename file '%s' to '%s': %s" % \ - (tmppath, filepath, e[1]), OfflineImapError.ERROR.FOLDER) + (tmppath, filepath, e[1]), OfflineImapError.ERROR.FOLDER), \ + None, exc_info()[2] if rtime != None: os.utime(filepath, (rtime, rtime)) diff --git a/offlineimap/folder/IMAP.py b/offlineimap/folder/IMAP.py index 52493fc..e67cb06 100644 --- a/offlineimap/folder/IMAP.py +++ b/offlineimap/folder/IMAP.py @@ -594,7 +594,9 @@ class IMAPFolder(BaseFolder): "repository '%s' failed (abort). Server responded: %s\n" "Message content was: %s"% (msg_id, self, self.getrepository(), str(e), dbg_output), - OfflineImapError.ERROR.MESSAGE) + OfflineImapError.ERROR.MESSAGE), \ + None, exc_info()[2] + # XXX: is this still needed? self.ui.error(e, exc_info()[2]) except imapobj.error as e: # APPEND failed # If the server responds with 'BAD', append() @@ -604,8 +606,8 @@ class IMAPFolder(BaseFolder): imapobj = None raise OfflineImapError("Saving msg (%s) folder '%s', repo '%s'" "failed (error). Server responded: %s\nMessage content was: " - "%s"% (msg_id, self, self.getrepository(), str(e), dbg_output), - OfflineImapError.ERROR.MESSAGE) + "%s" % (msg_id, self, self.getrepository(), str(e), dbg_output), + OfflineImapError.ERROR.MESSAGE), None, exc_info()[2] # 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() @@ -676,6 +678,7 @@ class IMAPFolder(BaseFolder): imapobj = self.imapserver.acquireconnection() self.ui.error(e, exc_info()[2]) fails_left -= 1 + # self.ui.error() will show the original traceback if not fails_left: raise e if data == [None] or res_type != 'OK': diff --git a/offlineimap/folder/LocalStatus.py b/offlineimap/folder/LocalStatus.py index 5f3d32d..ec34965 100644 --- a/offlineimap/folder/LocalStatus.py +++ b/offlineimap/folder/LocalStatus.py @@ -15,6 +15,7 @@ # along with this program; if not, write to the Free Software # Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA +from sys import exc_info import os import threading @@ -79,7 +80,7 @@ class LocalStatusFolder(BaseFolder): errstr = "Corrupt line '%s' in cache file '%s'" % \ (line, self.filename) self.ui.warn(errstr) - raise ValueError(errstr) + raise ValueError(errstr), None, exc_info()[2] self.messagelist[uid] = self.msglist_item_initializer(uid) self.messagelist[uid]['flags'] = flags @@ -103,7 +104,7 @@ class LocalStatusFolder(BaseFolder): errstr = "Corrupt line '%s' in cache file '%s'" % \ (line, self.filename) self.ui.warn(errstr) - raise ValueError(errstr) + raise ValueError(errstr), None, exc_info()[2] self.messagelist[uid] = self.msglist_item_initializer(uid) self.messagelist[uid]['flags'] = flags self.messagelist[uid]['mtime'] = mtime diff --git a/offlineimap/folder/LocalStatusSQLite.py b/offlineimap/folder/LocalStatusSQLite.py index a9ef6c2..1b2adb0 100644 --- a/offlineimap/folder/LocalStatusSQLite.py +++ b/offlineimap/folder/LocalStatusSQLite.py @@ -16,6 +16,7 @@ # Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA import os import re +from sys import exc_info from threading import Lock from .Base import BaseFolder try: @@ -66,7 +67,7 @@ class LocalStatusSQLiteFolder(BaseFolder): except NameError: # sqlite import had failed raise UserWarning('SQLite backend chosen, but no sqlite python ' - 'bindings available. Please install.') + 'bindings available. Please install.'), None, exc_info()[2] #Make sure sqlite is in multithreading SERIALIZE mode assert sqlite.threadsafety == 1, 'Your sqlite is not multithreading safe.' diff --git a/offlineimap/folder/Maildir.py b/offlineimap/folder/Maildir.py index 88ece8f..af1e5ff 100644 --- a/offlineimap/folder/Maildir.py +++ b/offlineimap/folder/Maildir.py @@ -19,6 +19,7 @@ import socket import time import re import os +from sys import exc_info from .Base import BaseFolder from threading import Lock try: @@ -282,7 +283,7 @@ class MaildirFolder(BaseFolder): continue severity = OfflineImapError.ERROR.MESSAGE raise OfflineImapError("Unique filename %s already exists." % \ - filename, severity) + filename, severity), None, exc_info()[2] else: raise @@ -372,7 +373,8 @@ class MaildirFolder(BaseFolder): except OSError as e: raise OfflineImapError("Can't rename file '%s' to '%s': %s" % ( oldfilename, newfilename, e[1]), - OfflineImapError.ERROR.FOLDER) + OfflineImapError.ERROR.FOLDER), \ + None, exc_info()[2] self.messagelist[uid]['flags'] = flags self.messagelist[uid]['filename'] = newfilename diff --git a/offlineimap/folder/UIDMaps.py b/offlineimap/folder/UIDMaps.py index 7815793..e1a6f5f 100644 --- a/offlineimap/folder/UIDMaps.py +++ b/offlineimap/folder/UIDMaps.py @@ -14,6 +14,8 @@ # You should have received a copy of the GNU General Public License # along with this program; if not, write to the Free Software # Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA + +from sys import exc_info from threading import Lock from offlineimap import OfflineImapError from .IMAP import IMAPFolder @@ -60,7 +62,7 @@ class MappedIMAPFolder(IMAPFolder): line = line.strip() except ValueError: raise Exception("Corrupt line '%s' in UID mapping file '%s'"% - (line, mapfilename)) + (line, mapfilename)), None, exc_info()[2] (str1, str2) = line.split(':') loc = long(str1) rem = long(str2) @@ -89,7 +91,7 @@ class MappedIMAPFolder(IMAPFolder): raise OfflineImapError("Could not find UID for msg '{0}' (f:'{1}'." " This is usually a bad thing and should be reported on the ma" "iling list.".format(e.args[0], self), - OfflineImapError.ERROR.MESSAGE) + OfflineImapError.ERROR.MESSAGE), None, exc_info()[2] # Interface from BaseFolder def cachemessagelist(self): diff --git a/offlineimap/imaplibutil.py b/offlineimap/imaplibutil.py index e012a01..917c726 100644 --- a/offlineimap/imaplibutil.py +++ b/offlineimap/imaplibutil.py @@ -18,6 +18,7 @@ import os import fcntl import time import subprocess +from sys import exc_info import threading from hashlib import sha1 @@ -54,7 +55,7 @@ class UsefulIMAPMixIn(object): errstr = "Server '%s' closed connection, error on SELECT '%s'. Ser"\ "ver said: %s" % (self.host, mailbox, e.args[0]) severity = OfflineImapError.ERROR.FOLDER_RETRY - raise OfflineImapError(errstr, severity) + raise OfflineImapError(errstr, severity), None, exc_info()[2] if result[0] != 'OK': #in case of error, bail out with OfflineImapError errstr = "Error SELECTing mailbox '%s', server reply:\n%s" %\ diff --git a/offlineimap/imapserver.py b/offlineimap/imapserver.py index 5601033..bac681e 100644 --- a/offlineimap/imapserver.py +++ b/offlineimap/imapserver.py @@ -206,8 +206,8 @@ class IMAPServer: imapobj.starttls() except imapobj.error as e: raise OfflineImapError("Failed to start " - "TLS connection: %s"% str(e), - OfflineImapError.ERROR.REPO) + "TLS connection: %s" % str(e), + OfflineImapError.ERROR.REPO, None, exc_info()[2]) ## All __authn_* procedures are helpers that do authentication. @@ -466,7 +466,7 @@ class IMAPServer: "'%s'. Make sure you have configured the ser"\ "ver name correctly and that you are online."%\ (self.hostname, self.repos) - raise OfflineImapError(reason, severity) + raise OfflineImapError(reason, severity), None, exc_info()[2] elif isinstance(e, SSLError) and e.errno == 1: # SSL unknown protocol error @@ -479,7 +479,7 @@ class IMAPServer: reason = "Unknown SSL protocol connecting to host '%s' for "\ "repository '%s'. OpenSSL responded:\n%s"\ % (self.hostname, self.repos, e) - raise OfflineImapError(reason, severity) + raise OfflineImapError(reason, severity), None, exc_info()[2] elif isinstance(e, socket.error) and e.args[0] == errno.ECONNREFUSED: # "Connection refused", can be a non-existing port, or an unauthorized @@ -487,15 +487,15 @@ class IMAPServer: reason = "Connection to host '%s:%d' for repository '%s' was "\ "refused. Make sure you have the right host and port "\ "configured and that you are actually able to access the "\ - "network."% (self.hostname, self.port, self.repos) - raise OfflineImapError(reason, severity) + "network." % (self.hostname, self.port, self.repos) + raise OfflineImapError(reason, severity), None, exc_info()[2] # Could not acquire connection to the remote; # socket.error(last_error) raised if str(e)[:24] == "can't open socket; error": raise OfflineImapError("Could not connect to remote server '%s' "\ "for repository '%s'. Remote does not answer." % (self.hostname, self.repos), - OfflineImapError.ERROR.REPO) + OfflineImapError.ERROR.REPO), None, exc_info()[2] else: # re-raise all other errors raise @@ -702,7 +702,7 @@ class IdleThread(object): self.ui.error(e, exc_info()[2]) self.parent.releaseconnection(imapobj, True) else: - raise e + raise else: success = True if "IDLE" in imapobj.capabilities: diff --git a/offlineimap/repository/IMAP.py b/offlineimap/repository/IMAP.py index d664259..2baa046 100644 --- a/offlineimap/repository/IMAP.py +++ b/offlineimap/repository/IMAP.py @@ -103,7 +103,8 @@ class IMAPRepository(BaseRepository): except Exception as e: raise OfflineImapError("remotehosteval option for repository "\ "'%s' failed:\n%s" % (self, e), - OfflineImapError.ERROR.REPO) + OfflineImapError.ERROR.REPO), \ + None, exc_info()[2] if host: self._host = host return self._host diff --git a/offlineimap/repository/__init__.py b/offlineimap/repository/__init__.py index 93861c2..076255e 100644 --- a/offlineimap/repository/__init__.py +++ b/offlineimap/repository/__init__.py @@ -15,6 +15,8 @@ # along with this program; if not, write to the Free Software # Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA +from sys import exc_info + try: from configparser import NoSectionError except ImportError: #python2 @@ -66,14 +68,16 @@ class Repository(object): except NoSectionError as e: errstr = ("Could not find section '%s' in configuration. Required " "for account '%s'." % ('Repository %s' % name, account)) - raise OfflineImapError(errstr, OfflineImapError.ERROR.REPO) + raise OfflineImapError(errstr, OfflineImapError.ERROR.REPO), \ + None, exc_info()[2] try: repo = typemap[repostype] except KeyError: errstr = "'%s' repository not supported for '%s' repositories." \ % (repostype, reqtype) - raise OfflineImapError(errstr, OfflineImapError.ERROR.REPO) + raise OfflineImapError(errstr, OfflineImapError.ERROR.REPO), \ + None, exc_info()[2] return repo(name, account) -- 1.9.0