We forgot to close files which fell out the lrucache

The initializer now takes a dispose function.  lrucache
claims ownership of the items it contains and will dispose
deleted items.  Ownership can naturally be reclaimed by calling
pop() for the item.

attic/lrucache.py

@@ -1,15 +1,16 @@
 class LRUCache(dict):
 
-    def __init__(self, capacity):
+    def __init__(self, capacity, dispose):
         super(LRUCache, self).__init__()
         self._lru = []
         self._capacity = capacity
+        self._dispose = dispose
 
     def __setitem__(self, key, value):
-        try:
-            self._lru.remove(key)
-        except ValueError:
-            pass
+        assert key not in self, (
+            "Unexpected attempt to replace a cached item."
+            " If this is intended, please delete or pop the old item first."
+            " The dispose function will be called on delete (but not pop).")
         self._lru.append(key)
         while len(self._lru) > self._capacity:
             del self[self._lru[0]]
@@ -28,7 +29,11 @@ class LRUCache(dict):
             self._lru.remove(key)
         except ValueError:
             pass
-        return super(LRUCache, self).__delitem__(key)
+        error = KeyError(key)
+        removed = super(LRUCache, self).pop(key, error)
+        if removed == error:
+            raise error
+        self._dispose(removed)
 
     def pop(self, key, default=None):
         try:
@@ -37,6 +42,11 @@ class LRUCache(dict):
             pass
         return super(LRUCache, self).pop(key, default)
 
+    def clear(self):
+        for value in self.values():
+            self._dispose(value)
+        super(LRUCache, self).clear()
+
     def _not_implemented(self, *args, **kw):
         raise NotImplementedError
     popitem = setdefault = update = _not_implemented

attic/repository.py

@@ -393,7 +393,8 @@ class LoggedIO(object):
 
     def __init__(self, path, limit, segments_per_dir, capacity=90):
         self.path = path
-        self.fds = LRUCache(capacity)
+        self.fds = LRUCache(capacity,
+                            dispose=lambda fd: fd.close())
         self.segment = 0
         self.limit = limit
         self.segments_per_dir = segments_per_dir
@@ -401,9 +402,8 @@ class LoggedIO(object):
         self._write_fd = None
 
     def close(self):
-        for segment in list(self.fds.keys()):
-            self.fds.pop(segment).close()
         self.close_segment()
+        self.fds.clear()
         self.fds = None  # Just to make sure we're disabled
 
     def segment_iterator(self, reverse=False):
@@ -477,9 +477,8 @@ class LoggedIO(object):
             return fd
 
     def delete_segment(self, segment):
-        fd = self.fds.pop(segment)
-        if fd is not None:
-            fd.close()
+        if segment in self.fds:
+            del self.fds[segment]
         try:
             os.unlink(self.segment_filename(segment))
         except OSError:
@@ -515,9 +514,8 @@ class LoggedIO(object):
             header = fd.read(self.header_fmt.size)
 
     def recover_segment(self, segment, filename):
-        fd = self.fds.pop(segment)
-        if fd is not None:
-            fd.close()
+        if segment in self.fds:
+            del self.fds[segment]
         # FIXME: save a copy of the original file
         with open(filename, 'rb') as fd:
             data = memoryview(fd.read())

attic/testsuite/lrucache.py

@@ -5,7 +5,7 @@ from attic.testsuite import AtticTestCase
 class LRUCacheTestCase(AtticTestCase):
 
     def test(self):
-        c = LRUCache(2)
+        c = LRUCache(2, dispose=lambda _: None)
         self.assert_equal(len(c), 0)
         for i, x in enumerate('abc'):
             c[x] = i
@@ -21,19 +21,13 @@ class LRUCacheTestCase(AtticTestCase):
         self.assert_equal(len(c), 2)
         self.assert_equal(c['c'], 2)
         self.assert_equal(c['d'], 3)
-        c['c'] = 22
-        c['e'] = 4
-        self.assert_equal(len(c), 2)
-        self.assert_raises(KeyError, lambda: c['d'])
-        self.assert_equal(c['c'], 22)
-        self.assert_equal(c['e'], 4)
         del c['c']
         self.assert_equal(len(c), 1)
         self.assert_raises(KeyError, lambda: c['c'])
-        self.assert_equal(c['e'], 4)
+        self.assert_equal(c['d'], 3)
 
     def test_pop(self):
-        c = LRUCache(2)
+        c = LRUCache(2, dispose=lambda _: None)
         c[1] = 1
         c[2] = 2
         c.pop(1)