mbox locking: combine locking functions into one and swap lock order

We used to create a dotlock file first and then lock with fcntl; swap that
order, since locking first with fcntl seems to be more common.

This patch also adds general mbox lock/unlock methods, which call the
dotlock and fcntl-lock methods, and moves the retry logic there.

When the dotlock and fcntl methods fail to acquire a lock, they now raise
a custom exception "LockUnavailable", which gets caught in the general
lock() method.  That way, if we succeed to acquire one lock but fail to
acquire the other, we can release our locks at the upper level and retry.

archivemail.py

@@ -80,6 +80,8 @@ class UserError(ArchivemailException):
     pass
 class UnexpectedError(ArchivemailException): 
     pass
+class LockUnavailable(ArchivemailException):
+    pass
 
 class Stats:
     """Class to collect and print statistics about mailbox archival"""
@@ -177,9 +179,9 @@ class Options:
     dry_run              = 0
     filter_append        = None
     include_flagged      = 0
-    lockfile_attempts    = 5  
+    locking_attempts     = 5
     lockfile_extension   = ".lock"
-    lockfile_sleep       = 1 
+    lock_sleep           = 1
     no_compress          = 0
     only_archive_read    = 0
     output_dir           = None
@@ -328,6 +330,7 @@ class Mbox(mailbox.UnixMailbox):
         path -- file name of the 'mbox' file to be opened
         """
         assert(path)
+        self._locked = False
         try:
             self.original_atime = os.path.getatime(path)
             self.original_mtime = os.path.getmtime(path)
@@ -351,18 +354,54 @@ class Mbox(mailbox.UnixMailbox):
         os.utime(self.mbox_file_name, (self.original_atime,  \
             self.original_mtime)) 
 
-    def posix_lock(self):
+    def lock(self):
-        """Set an exclusive posix lock on the 'mbox' mailbox"""
+        """Lock this mbox with both a dotlock and a posix lock."""
-        vprint("obtaining exclusive lock on file '%s'" % self.mbox_file_name)
+        assert(not self._locked)
-        fcntl.lockf(self.mbox_file, fcntl.LOCK_EX|fcntl.LOCK_NB)
+        attempt = 1
+        while True:
+            try:
+                self._posix_lock()
+                self._dotlock_lock()
+                break
+            except LockUnavailable, e:
+                self._posix_unlock()
+                attempt += 1
+                if (attempt > options.locking_attempts):
+                    unexpected_error(str(e))
+                vprint("%s - sleeping..." % e)
+                time.sleep(options.lock_sleep)
+            except:
+                self._posix_unlock()
+                raise
+        self._locked = True
 
-    def posix_unlock(self):
+    def unlock(self):
+        """Unlock this mbox."""
+        assert(self._locked)
+        self._dotlock_unlock()
+        self._posix_unlock()
+        self._locked = False
+
+    def _posix_lock(self):
+        """Set an exclusive posix lock on the 'mbox' mailbox"""
+        vprint("trying to acquire posix lock on file '%s'" % self.mbox_file_name)
+        try:
+            fcntl.lockf(self.mbox_file, fcntl.LOCK_EX|fcntl.LOCK_NB)
+        except IOError, e:
+            if e.errno in (errno.EAGAIN, errno.EACCES):
+                raise LockUnavailable("posix lock for '%s' unavailable" % \
+                    self.mbox_file_name)
+            else:
+                raise
+        vprint("acquired posix lock on file '%s'" % self.mbox_file_name)
+
+    def _posix_unlock(self):
         """Unset any posix lock on the 'mbox' mailbox"""
         vprint("dropping posix lock on file '%s'" % self.mbox_file_name)
         fcntl.lockf(self.mbox_file, fcntl.LOCK_UN)
 
-    def dotlock_lock(self):
+    def _dotlock_lock(self):
-        """Create a dotlock file on the 'mbox' mailbox"""
+        """Create a dotlock file for the 'mbox' mailbox"""
         import socket
         hostname = socket.gethostname()
         pid = os.getpid()
@@ -371,36 +410,29 @@ class Mbox(mailbox.UnixMailbox):
         plfd, prelock_name = tempfile.mkstemp(prelock_suffix, prelock_prefix,
             dir=box_dir)
         lock_name = self.mbox_file_name + options.lockfile_extension
-        attempt = 0
         try:
-            while True:
+            try:
-                attempt = attempt + 1
+                os.link(prelock_name, lock_name)
-                try:
+                # We've got the lock.
-                    os.link(prelock_name, lock_name)
+            except OSError, e:
-                    # We've got the lock.
+                if os.fstat(plfd)[stat.ST_NLINK] == 2:
-                    break
+                    # The Linux man page for open(2) claims that in this
-                except OSError, e:
+                    # case we have actually succeeded to create the link,
-                    if os.fstat(plfd)[stat.ST_NLINK] == 2:
+                    # and this assumption seems to be folklore.
-                        # The Linux man page for open(2) claims that in this
+                    # So we've got the lock.
-                        # case we have actually succeeded to create the link, 
+                    pass
-                        # and this assumption seems to be folklore. 
+                elif e.errno == errno.EEXIST:
-                        # So we've got the lock.
+                    raise LockUnavailable("Dotlock for '%s' unavailable" % self.mbox_file_name)
-                        break
+                else:
-                    if e.errno != errno.EEXIST: raise
+                    raise
-                    # Lockfile already existed, someone else has the lock.
-                    if (attempt >= options.lockfile_attempts):
-                        unexpected_error("Giving up waiting for "
-                            "dotlock '%s'" % lock_name)
-                    vprint("lockfile '%s' exists - sleeping..." % lock_name)
-                    time.sleep(options.lockfile_sleep)
         finally:
             os.close(plfd)
             os.unlink(prelock_name)
         _stale.dotlock_lock = lock_name
         vprint("acquired lockfile '%s'" % lock_name)
 
-    def dotlock_unlock(self):
+    def _dotlock_unlock(self):
-        """Delete the dotlock file on the 'mbox' mailbox"""
+        """Delete the dotlock file for the 'mbox' mailbox."""
         assert(self.mbox_file_name)
         lock_name = self.mbox_file_name + options.lockfile_extension
         vprint("removing lockfile '%s'" % lock_name)
@@ -1132,8 +1164,7 @@ def _archive_mbox(mailbox_name, final_archive_name):
     else:
         archive = ArchiveMbox(final_archive_name)
 
-    original.dotlock_lock()
+    original.lock()
-    original.posix_lock()
     msg = original.next()
     if not msg and (original.starting_size > 0):
         user_error("'%s' is not a valid mbox-format mailbox" % mailbox_name)
@@ -1164,10 +1195,9 @@ def _archive_mbox(mailbox_name, final_archive_name):
         archive.finalise()
     if retain:
         retain.finalise()
-    original.posix_unlock()
+    original.unlock()
     original.close()
     original.reset_timestamps()
-    original.dotlock_unlock()
     if not options.quiet:
         stats.display()
 

test_archivemail.py

@@ -115,9 +115,9 @@ class TestMboxDotlock(TestCaseInTempdir):
     def testDotlock(self):
         """dotlock_lock/unlock should create/delete a lockfile"""
         lock = self.mbox_name + ".lock"
-        self.mbox.dotlock_lock()
+        self.mbox._dotlock_lock()
         assert(os.path.isfile(lock))
-        self.mbox.dotlock_unlock()
+        self.mbox._dotlock_unlock()
         assert(not os.path.isfile(lock))
 
 class TestMboxPosixLock(TestCaseInTempdir):
@@ -138,9 +138,9 @@ class TestMboxPosixLock(TestCaseInTempdir):
         pid = os.fork()
         if pid == 0:
             # In the child, lock the mailbox.
-            self.mbox.posix_lock()
+            self.mbox._posix_lock()
             time.sleep(2)
-            self.mbox.posix_unlock()
+            self.mbox._posix_unlock()
             os._exit(0)
 
         # In the parent, sleep a bit to give the child time to acquire