More on C and ZODB compatibility for Python PickleCache and Persisten…

…t. There are some interconnected lifetime issues that this solves for ZODB's cache tests, and what appear to be some genuine bugs in invalidation.

.gitignore

@@ -14,3 +14,4 @@ coverage.xml
 *.egg-info
 .installed.cfg
 .dir-locals.el
+dist

persistent/persistence.py

@@ -350,15 +350,26 @@ def _p_activate(self):
                     _OSA(self, '_Persistent__flags', before)
                     raise
 
+    # In the C implementation, _p_invalidate winds up calling
+    # _p_deactivate. There are ZODB tests that depend on this;
+    # it's not documented but there may be code in the wild
+    # that does as well
+
     def _p_deactivate(self):
         """ See IPersistent.
         """
         if self.__flags is not None and not self.__flags:
-            self._p_invalidate()
+            self._p_invalidate_deactivate_helper()
 
     def _p_invalidate(self):
         """ See IPersistent.
         """
+        # If we think we have changes, we must pretend
+        # like we don't so that deactivate does its job
+        _OSA(self, '_Persistent__flags', 0)
+        self._p_deactivate()
+
+    def _p_invalidate_deactivate_helper(self):
         if self.__jar is not None:
             if self.__flags is not None:
                 _OSA(self, '_Persistent__flags', None)
@@ -374,6 +385,8 @@ def _p_invalidate(self):
             if cache is not None:
                 cache.update_object_size_estimation(self.__oid,
                                                     -1)
+                # See notes in PickleCache.sweep for why we have to do this
+                cache._persistent_deactivate_ran = True
 
     def _p_getattr(self, name):
         """ See IPersistent.

persistent/picklecache.py

@@ -22,6 +22,7 @@
 from persistent.interfaces import IPickleCache
 from persistent.interfaces import STICKY
 from persistent.interfaces import OID_TYPE
+from persistent.interfaces import UPTODATE
 from persistent import Persistent
 
 # Tests may modify this to add additional types
@@ -35,12 +36,23 @@ def __init__(self, object, next=None, prev=None):
         self.next = next
         self.prev = prev
 
+def _sweeping_ring(f):
+    def locked(self, *args, **kwargs):
+        self._is_sweeping_ring = True
+        try:
+            f(self, *args, **kwargs)
+        finally:
+            self._is_sweeping_ring = False
+    return locked
+
 @implementer(IPickleCache)
 class PickleCache(object):
 
     total_estimated_size = 0
     cache_size_bytes = 0
 
+    _is_sweeping_ring = False
+
     def __init__(self, jar, target_size=0, cache_size_bytes=0):
         # TODO:  forward-port Dieter's bytes stuff
         self.jar = jar
@@ -111,11 +123,7 @@ def __setitem__(self, oid, value):
             self.persistent_classes[oid] = value
         else:
             self.data[oid] = value
-            if value._p_state != GHOST:
-                self.non_ghost_count += 1
-                mru = self.ring.prev
-                self.ring.prev = node = RingNode(value, self.ring, mru)
-                mru.next = node
+            self.mru(oid)
 
     def __delitem__(self, oid):
         """ See IPickleCache.
@@ -137,6 +145,7 @@ def __delitem__(self, oid):
     def get(self, oid, default=None):
         """ See IPickleCache.
         """
+
         value = self.data.get(oid, self)
         if value is not self:
             return value
@@ -145,6 +154,11 @@ def get(self, oid, default=None):
     def mru(self, oid):
         """ See IPickleCache.
         """
+        if self._is_sweeping_ring:
+            # accessess during sweeping, such as with an
+            # overridden _p_deactivate, don't mutate the ring
+            # because that could leave it inconsistent
+            return
         node = self.ring.next
         while node is not self.ring and node.object._p_oid != oid:
             node = node.next
@@ -156,6 +170,7 @@ def mru(self, oid):
                 self.ring.prev = node = RingNode(value, self.ring, mru)
                 mru.next = node
         else:
+            assert node.object._p_oid == oid
             # remove from old location
             node.prev.next, node.next.prev = node.next, node.prev
             # splice into new
@@ -195,10 +210,10 @@ def klass_items(self):
     def incrgc(self, ignored=None):
         """ See IPickleCache.
         """
-        target = self.target_size
+        target = self.cache_size
         if self.drain_resistance >= 1:
             size = self.non_ghost_count
-            target2 = size - 1 - (size / self.drain_resistance)
+            target2 = size - 1 - (size // self.drain_resistance)
             if target2 < target:
                 target = target2
         self._sweep(target, self.cache_size_bytes)
@@ -287,27 +302,53 @@ def update_object_size_estimation(self, oid, new_size):
     cache_klass_count = property(lambda self: len(self.persistent_classes))
 
     # Helpers
+
+    # Set to true when a deactivation happens in our code. For
+    # compatibility with the C implementation, we can only remove the
+    # node and decrement our non-ghost count if our implementation
+    # actually runs (broken subclasses can forget to call super; ZODB
+    # has tests for this). This gets set to false everytime we examine
+    # a node and checked afterwards. The C implementation has a very
+    # incestuous relatiounship between cPickleCache and cPersistence:
+    # the pickle cache calls _p_deactivate, which is responsible for
+    # both decrementing the non-ghost count and removing its node from
+    # the cache ring. We're trying to keep that to a minimum, but
+    # there's no way around it if we want full compatibility
+    _persistent_deactivate_ran = False
+
+    @_sweeping_ring
     def _sweep(self, target, target_size_bytes=0):
         # lock
         node = self.ring.next
         ejected = 0
+
         while (node is not self.ring
-               and (self.non_ghost_count > target
+               and ( self.non_ghost_count > target
                     or (target_size_bytes and self.total_estimated_size > target_size_bytes))):
-            if node.object._p_state not in (STICKY, CHANGED):
-                ejected += 1
-                node.prev.next, node.next.prev = node.next, node.prev
+            if node.object._p_state == UPTODATE:
+                # The C implementation will only evict things that are specifically
+                # in the up-to-date state
+                self._persistent_deactivate_ran = False
+
                 # sweeping an object out of the cache should also
                 # ghost it---that's what C does. This winds up
                 # calling `update_object_size_estimation`.
                 # Also in C, if this was the last reference to the object,
                 # it removes itself from the `data` dictionary.
                 # If we're under PyPy or Jython, we need to run a GC collection
                 # to make this happen...this is only noticeable though, when
-                # we eject objects
+                # we eject objects. Also, note that we can only take any of these
+                # actions if our _p_deactivate ran, in case of buggy subclasses.
+                # see _persistent_deactivate_ran
+
                 node.object._p_deactivate()
-                node.object = None
-                self.non_ghost_count -= 1
+                if (self._persistent_deactivate_ran
+                    # Test-cases sneak in non-Persistent objects, sigh, so naturally
+                    # they don't cooperate (without this check a bunch of test_picklecache
+                    #breaks)
+                    or not isinstance(node.object, Persistent)):
+                    ejected += 1
+                    self.__remove_from_ring(node)
             node = node.next
         if ejected:
             # TODO: Only do this on PyPy/Jython?
@@ -316,21 +357,26 @@ def _sweep(self, target, target_size_bytes=0):
             gc.collect()
         return ejected
 
+    @_sweeping_ring
     def _invalidate(self, oid):
         value = self.data.get(oid)
         if value is not None and value._p_state != GHOST:
             value._p_invalidate()
             node = self.ring.next
-            while True:
-                if node is self.ring:
-                    break  # pragma: no cover belt-and-suspenders
+            while node is not self.ring:
                 if node.object is value:
-                    node.prev.next, node.next.prev = node.next, node.prev
+                    self.__remove_from_ring(node)
                     break
                 node = node.next
         elif oid in self.persistent_classes:
             del self.persistent_classes[oid]
 
+    def __remove_from_ring(self, node):
+        "Take the node, which previously contained a non-ghost, out of the ring"
+        node.object = None
+        node.prev.next, node.next.prev = node.next, node.prev
+        self.non_ghost_count -= 1
+
 def _estimated_size_in_24_bits(value):
     if value > 1073741696:
         return 16777215