[Python-checkins] r52776 - in python/trunk/Lib: mailbox.py test/test_mailbox.py

[andrew.kuchling]

Author: andrew.kuchling
Date: Fri Nov 17 14:30:25 2006
New Revision: 52776

Modified:
   python/trunk/Lib/mailbox.py
   python/trunk/Lib/test/test_mailbox.py
Log:
Remove file-locking in MH.pack() method.
This change looks massive but it's mostly a re-indenting after
removing some try...finally blocks.

Also adds a test case that does a pack() while the mailbox is locked; this 
test would have turned up bugs in the original code on some platforms.

In both nmh and GNU Mailutils' implementation of MH-format mailboxes,
no locking is done of individual message files when renaming them.

The original mailbox.py code did do locking, which meant that message
files had to be opened.  This code was buggy on certain platforms
(found through reading the code); there were code paths that closed
the file object and then called _unlock_file() on it.

Will backport to 25-maint once I see how the buildbots react to this patch.


Modified: python/trunk/Lib/mailbox.py
==============================================================================
--- python/trunk/Lib/mailbox.py	(original)
+++ python/trunk/Lib/mailbox.py	Fri Nov 17 14:30:25 2006
@@ -1054,27 +1054,13 @@
         for key in self.iterkeys():
             if key - 1 != prev:
                 changes.append((key, prev + 1))
-                f = open(os.path.join(self._path, str(key)), 'r+')
-                try:
-                    if self._locked:
-                        _lock_file(f)
-                    try:
-                        if hasattr(os, 'link'):
-                            os.link(os.path.join(self._path, str(key)),
-                                    os.path.join(self._path, str(prev + 1)))
-                            if sys.platform == 'os2emx':
-                                # cannot unlink an open file on OS/2
-                                f.close()
-                            os.unlink(os.path.join(self._path, str(key)))
-                        else:
-                            f.close()
-                            os.rename(os.path.join(self._path, str(key)),
-                                      os.path.join(self._path, str(prev + 1)))
-                    finally:
-                        if self._locked:
-                            _unlock_file(f)
-                finally:
-                    f.close()
+                if hasattr(os, 'link'):
+                    os.link(os.path.join(self._path, str(key)),
+                            os.path.join(self._path, str(prev + 1)))
+                    os.unlink(os.path.join(self._path, str(key)))
+                else:
+                    os.rename(os.path.join(self._path, str(key)),
+                              os.path.join(self._path, str(prev + 1)))
             prev += 1
         self._next_key = prev + 1
         if len(changes) == 0:

Modified: python/trunk/Lib/test/test_mailbox.py
==============================================================================
--- python/trunk/Lib/test/test_mailbox.py	(original)
+++ python/trunk/Lib/test/test_mailbox.py	Fri Nov 17 14:30:25 2006
@@ -887,6 +887,21 @@
         self.assert_(self._box.get_sequences() ==
                      {'foo':[1, 2, 3], 'unseen':[1], 'bar':[3], 'replied':[3]})
 
+        # Test case for packing while holding the mailbox locked.
+        key0 = self._box.add(msg1)
+        key1 = self._box.add(msg1)
+        key2 = self._box.add(msg1)
+        key3 = self._box.add(msg1)
+
+        self._box.remove(key0)
+        self._box.remove(key2)
+        self._box.lock()
+        self._box.pack()
+        self._box.unlock()
+        self.assert_(self._box.get_sequences() ==
+                     {'foo':[1, 2, 3, 4, 5],
+                      'unseen':[1], 'bar':[3], 'replied':[3]})
+        
     def _get_lock_path(self):
         return os.path.join(self._path, '.mh_sequences.lock')