Merge lp:~jameinel/bzr/1.17-lru-cleanup into lp:~bzr/bzr/trunk-old

[John A Meinel]

[John A Meinel (jameinel)]

Just a simple cleanup to fix bug #396636

For LRUCache, if you register a cleanup function, that function raising an exception can mess up the linked list.

This just moves the linked list management into a try/finally block.

[Vincent Ladeuil (vila)]

 review needs_fixing
 merge approved

>>>>> "jam" == John A Meinel <email address hidden> writes:

    jam> class LRUCache(object):
    jam> @@ -156,13 +158,16 @@
    jam> raise ValueError('cannot use _null_key as a key')
    jam> if key in self._cache:
    jam> node = self._cache[key]
    jam> - node.run_cleanup()
    jam> - node.value = value
    jam> - node.cleanup = cleanup
    jam> + try:
    jam> + node.run_cleanup()
    jam> + finally:
    jam> + node.value = value
    jam> + node.cleanup = cleanup
    jam> + self._record_access(node)
    jam> else:
    jam> node = _LRUNode(key, value, cleanup=cleanup)
    jam> self._cache[key] = node
    jam> - self._record_access(node)
    jam> + self._record_access(node)

Don't duplicate that line and add a comment in the finally block,
please.

review: Needs Fixing

=== modified file 'NEWS'
--- NEWS	2009-07-08 18:05:38 +0000
+++ NEWS	2009-07-08 19:35:13 +0000
@@ -82,12 +82,8 @@
 * Force socket shutdown in threaded http test servers to avoid client hangs
   (pycurl).  (Vincent Ladeuil, #383920).
 
-* The ``log+`` decorator, useful in debugging or profiling, could cause
-  "AttributeError: 'list' object has no attribute 'next'".  This is now
-  fixed.  The log decorator no longer shows the elapsed time or transfer
-  rate because they're available in the log prefixes and the transport
-  activity display respectively.
-  (Martin Pool, #340347)
+* ``LRUCache`` will maintain the linked list pointers even if a nodes
+  cleanup function raises an exception. (John Arbash Meinel, #396838)
 
 * Progress bars are now suppressed again when the environment variable
   ``BZR_PROGRESS_BAR`` is set to ``none``.
@@ -110,6 +106,13 @@
   removing the stacking location inside ``.bzr/branch``.
   (Robert Collins, #376243)
 
+* The ``log+`` decorator, useful in debugging or profiling, could cause
+  "AttributeError: 'list' object has no attribute 'next'".  This is now
+  fixed.  The log decorator no longer shows the elapsed time or transfer
+  rate because they're available in the log prefixes and the transport
+  activity display respectively.
+  (Martin Pool, #340347)
+
 * Unshelve works correctly when multiple zero-length files are present on
   the shelf. (Aaron Bentley, #363444)
 

=== modified file 'bzrlib/lru_cache.py'
--- bzrlib/lru_cache.py	2009-05-01 20:20:37 +0000
+++ bzrlib/lru_cache.py	2009-07-08 19:35:14 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2006, 2008 Canonical Ltd
+# Copyright (C) 2006, 2008, 2009 Canonical Ltd
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -48,11 +48,14 @@
                                      self.next_key, prev_key)
 
     def run_cleanup(self):
-        if self.cleanup is not None:
-            self.cleanup(self.key, self.value)
-        self.cleanup = None
-        # Just make sure to break any refcycles, etc
-        self.value = None
+        try:
+            if self.cleanup is not None:
+                self.cleanup(self.key, self.value)
+        finally:
+            # cleanup might raise an exception, but we want to make sure
+            # to break refcycles, etc
+            self.cleanup = None
+            self.value = None
 
 
 class LRUCache(object):
@@ -156,13 +159,18 @@
             raise ValueError('cannot use _null_key as a key')
         if key in self._cache:
             node = self._cache[key]
-            node.run_cleanup()
-            node.value = value
-            node.cleanup = cleanup
+            try:
+                node.run_cleanup()
+            finally:
+                # Maintain the LRU properties, even if cleanup raises an
+                # exception
+                node.value = value
+                node.cleanup = cleanup
+                self._record_access(node)
         else:
             node = _LRUNode(key, value, cleanup=cleanup)
             self._cache[key] = node
-        self._record_access(node)
+            self._record_access(node)
 
         if len(self._cache) > self._max_cache:
             # Trigger the cleanup
@@ -241,16 +249,19 @@
         # If we have removed all entries, remove the head pointer as well
         if self._least_recently_used is None:
             self._most_recently_used = None
-        node.run_cleanup()
-        # Now remove this node from the linked list
-        if node.prev is not None:
-            node.prev.next_key = node.next_key
-        if node.next_key is not _null_key:
-            node_next = self._cache[node.next_key]
-            node_next.prev = node.prev
-        # And remove this node's pointers
-        node.prev = None
-        node.next_key = _null_key
+        try:
+            node.run_cleanup()
+        finally:
+            # cleanup might raise an exception, but we want to make sure to
+            # maintain the linked list
+            if node.prev is not None:
+                node.prev.next_key = node.next_key
+            if node.next_key is not _null_key:
+                node_next = self._cache[node.next_key]
+                node_next.prev = node.prev
+            # And remove this node's pointers
+            node.prev = None
+            node.next_key = _null_key
 
     def _remove_lru(self):
         """Remove one entry from the lru, and handle consequences.

=== modified file 'bzrlib/tests/test_lru_cache.py'
--- bzrlib/tests/test_lru_cache.py	2009-04-16 22:06:25 +0000
+++ bzrlib/tests/test_lru_cache.py	2009-07-08 19:35:14 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2006, 2008 Canonical Ltd
+# Copyright (C) 2006, 2008, 2009 Canonical Ltd
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -132,6 +132,43 @@
         cache[2] = 26
         self.assertEqual([(2, 20), (2, 25)], cleanup_called)
 
+    def test_cleanup_error_maintains_linked_list(self):
+        cleanup_called = []
+        def cleanup_func(key, val):
+            cleanup_called.append((key, val))
+            raise ValueError('failure during cleanup')
+
+        cache = lru_cache.LRUCache(max_cache=10)
+        for i in xrange(10):
+            cache.add(i, i, cleanup=cleanup_func)
+        for i in xrange(10, 20):
+            self.assertRaises(ValueError,
+                cache.add, i, i, cleanup=cleanup_func)
+
+        self.assertEqual([(i, i) for i in xrange(10)], cleanup_called)
+
+        self.assertEqual(range(19, 9, -1), [n.key for n in cache._walk_lru()])
+
+    def test_cleanup_during_replace_still_replaces(self):
+        cleanup_called = []
+        def cleanup_func(key, val):
+            cleanup_called.append((key, val))
+            raise ValueError('failure during cleanup')
+
+        cache = lru_cache.LRUCache(max_cache=10)
+        for i in xrange(10):
+            cache.add(i, i, cleanup=cleanup_func)
+        self.assertRaises(ValueError,
+            cache.add, 1, 20, cleanup=cleanup_func)
+        # We also still update the recent access to this node
+        self.assertEqual([1, 9, 8, 7, 6, 5, 4, 3, 2, 0],
+                         [n.key for n in cache._walk_lru()])
+        self.assertEqual(20, cache[1])
+
+        self.assertEqual([(1, 1)], cleanup_called)
+        self.assertEqual([1, 9, 8, 7, 6, 5, 4, 3, 2, 0],
+                         [n.key for n in cache._walk_lru()])
+
     def test_len(self):
         cache = lru_cache.LRUCache(max_cache=10, after_cleanup_count=10)