Merge pull request #35 from NextThought/issue22

Update issue #22

CHANGES.rst

@@ -15,6 +15,10 @@
   and ``fsrecover`` among others). This requires the addition of the
   ``zodbpickle`` dependency.
 
+- Fix #21, FileStorage: an edge case when disk space runs out while packing,
+  do not leave the ``.pack`` file around. That would block any write to the
+  to-be-packed ``Data.fs``, because the disk would stay at 0 bytes free.
+
 4.1.0 (2015-01-11)
 ==================
 

src/ZODB/FileStorage/fspack.py

@@ -407,23 +407,43 @@ class FileStoragePacker(FileStorageFormatter):
 
         self.gc.findReachable()
 
+        def close_files_remove():
+            # blank except: we might be in an IOError situation/handler
+            # try our best, but don't fail
+            try:
+                self._tfile.close()
+            except:
+                pass
+            try:
+                self._file.close()
+            except:
+                pass
+            try:
+                os.remove(self._name + ".pack")
+            except:
+                pass
+            if self.blob_removed is not None:
+                self.blob_removed.close()
+
         # Setup the destination file and copy the metadata.
         # TODO:  rename from _tfile to something clearer.
         self._tfile = open(self._name + ".pack", "w+b")
+        try:
             self._file.seek(0)
             self._tfile.write(self._file.read(self._metadata_size))
 
             self._copier = PackCopier(self._tfile, self.index, self.tindex)
 
             ipos, opos = self.copyToPacktime()
+        except (OSError, IOError):
+            # most probably ran out of disk space or some other IO error
+            close_files_remove()
+            raise  # don't succeed silently
+
         assert ipos == self.gc.packpos
         if ipos == opos:
             # pack didn't free any data.  there's no point in continuing.
-            self._tfile.close()
-            self._file.close()
-            os.remove(self._name + ".pack")
-            if self.blob_removed is not None:
-                self.blob_removed.close()
+            close_files_remove()
             return None
         self._commit_lock_acquire()
         self.locked = True
@@ -462,6 +482,12 @@ class FileStoragePacker(FileStorageFormatter):
                 self.blob_removed.close()
 
             return pos
+        except (OSError, IOError):
+            # most probably ran out of disk space or some other IO error
+            close_files_remove()
+            if self.locked:
+                self._commit_lock_release()
+            raise  # don't succeed silently
         except:
             if self.locked:
                 self._commit_lock_release()

src/ZODB/FileStorage/tests.py

@@ -185,6 +185,125 @@ cleanup
 
     """
 
+def pack_disk_full_copyToPacktime():
+    """Recover from a disk full situation by removing the `.pack` file
+
+`copyToPacktime` fails
+
+Add some data
+
+    >>> fs = ZODB.FileStorage.FileStorage('data.fs')
+    >>> db = ZODB.DB(fs)
+    >>> conn = db.open()
+    >>> conn.root()[1] = 'foobar'
+    >>> transaction.commit()
+
+patch `copyToPacktime` to fail
+
+    >>> from ZODB.FileStorage import fspack
+    >>> save_copyToPacktime = fspack.FileStoragePacker.copyToPacktime
+
+    >>> def failing_copyToPacktime(self):
+    ...     self._tfile.write(b'somejunkdata')
+    ...     raise OSError("No space left on device")
+
+    >>> fspack.FileStoragePacker.copyToPacktime = failing_copyToPacktime
+
+pack -- it still raises `OSError`
+
+    >>> db.pack(time.time()+1)
+    Traceback (most recent call last):
+    ...
+    OSError: No space left on device
+
+`data.fs.pack` must not exist
+
+    >>> os.path.exists('data.fs.pack')
+    False
+
+undo patching
+
+    >>> fspack.FileStoragePacker.copyToPacktime = save_copyToPacktime
+
+    >>> db.close()
+
+check the data we added
+
+    >>> fs = ZODB.FileStorage.FileStorage('data.fs')
+    >>> db = ZODB.DB(fs)
+    >>> conn = db.open()
+    >>> conn.root()[1]
+    'foobar'
+    >>> db.close()
+    """
+
+def pack_disk_full_copyRest():
+    """Recover from a disk full situation by removing the `.pack` file
+
+`copyRest` fails
+
+Add some data
+
+    >>> fs = ZODB.FileStorage.FileStorage('data.fs')
+    >>> db = ZODB.DB(fs)
+    >>> conn = db.open()
+    >>> conn.root()[1] = 'foobar'
+    >>> transaction.commit()
+
+patch `copyToPacktime` to add one more transaction
+
+    >>> from ZODB.FileStorage import fspack
+    >>> save_copyToPacktime = fspack.FileStoragePacker.copyToPacktime
+
+    >>> def patched_copyToPacktime(self):
+    ...     res = save_copyToPacktime(self)
+    ...     conn2 = db.open()
+    ...     conn2.root()[2] = 'another bar'
+    ...     transaction.commit()
+    ...     return res
+
+    >>> fspack.FileStoragePacker.copyToPacktime = patched_copyToPacktime
+
+patch `copyRest` to fail
+
+    >>> save_copyRest = fspack.FileStoragePacker.copyRest
+
+    >>> def failing_copyRest(self, ipos):
+    ...     self._tfile.write(b'somejunkdata')
+    ...     raise OSError("No space left on device")
+
+    >>> fspack.FileStoragePacker.copyRest = failing_copyRest
+
+pack -- it still raises `OSError`
+
+    >>> db.pack(time.time()+1)
+    Traceback (most recent call last):
+    ...
+    OSError: No space left on device
+
+`data.fs.pack` must not exist
+
+    >>> os.path.exists('data.fs.pack')
+    False
+
+undo patching
+
+    >>> fspack.FileStoragePacker.copyToPacktime = save_copyToPacktime
+    >>> fspack.FileStoragePacker.copyRest = save_copyRest
+
+    >>> db.close()
+
+check the data we added
+
+    >>> fs = ZODB.FileStorage.FileStorage('data.fs')
+    >>> db = ZODB.DB(fs)
+    >>> conn = db.open()
+    >>> conn.root()[1]
+    'foobar'
+    >>> conn.root()[2]
+    'another bar'
+    >>> db.close()
+    """
 
 def test_suite():
     return unittest.TestSuite((
@@ -199,4 +318,3 @@ def test_suite():
             tearDown=ZODB.tests.util.tearDown,
             checker=checker),
         ))
-