Merge pull request #162 from zopefoundation/issue149

Make the Python PickleCache run a GC when it evicts weakly-referenced objects.

CHANGES.rst

@@ -16,6 +16,13 @@
   ``PURE_PYTHON=1``) to not print unraisable ``AttributeErrors`` from
   ``_WeakValueDictionary`` during garbage collection. See `issue 150
   <https://github.com/zopefoundation/persistent/issues/150>`_.
+- Make the pure-Python implementation of the cache run a garbage
+  collection (``gc.collect()``) on ``full_sweep``, ``incrgc`` and
+  ``minimize`` *if* it detects that an object that was weakly
+  referenced has been ejected. This solves issues on PyPy with ZODB raising
+  ``ConnectionStateError`` when there are persistent
+  ``zope.interface`` utilities/adapters registered. This partly
+  reverts a change from release 4.2.3.
 
 4.6.4 (2020-03-26)
 ==================

persistent/picklecache.py

@@ -430,6 +430,13 @@ def update_object_size_estimation(self, oid, new_size):
     @_sweeping_ring
     def _sweep(self, target, target_size_bytes=0):
         ejected = 0
+        # If we find and eject objects that may have been weak referenced,
+        # we need to run a garbage collection to try to clear those references.
+        # Otherwise, it's highly likely that accessing those objects through those
+        # references will try to ``_p_activate()`` them, and since the jar they came
+        # from is probably closed, that will lead to an error. See
+        # https://github.com/zopefoundation/persistent/issues/149
+        had_weak_refs = False
         ring = self.ring
         for node, value in ring.iteritems():
             if ((target or target_size_bytes) # pylint:disable=too-many-boolean-expressions
@@ -452,7 +459,10 @@ def _sweep(self, target, target_size_bytes=0):
                 # to make this happen...this is only noticeable though, when
                 # 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
+                # see _persistent_deactivate_ran.
+
+                if not had_weak_refs:
+                    had_weak_refs |= getattr(value, '__weakref__', None) is not None
 
                 value._p_deactivate()
                 if (self._persistent_deactivate_ran
@@ -464,6 +474,12 @@ def _sweep(self, target, target_size_bytes=0):
                     ejected += 1
                     self.non_ghost_count -= 1
 
+        if ejected and had_weak_refs:
+            # Clear the iteration variables, so the objects they point to
+            # are subject to GC.
+            node = None
+            value = None
+            gc.collect()
         return ejected
 
     @_sweeping_ring

persistent/tests/test_picklecache.py

@@ -44,6 +44,16 @@ class DummyConnection(object):
     pass
 
 
+class ClosedConnection(DummyConnection):
+    def __init__(self, test):
+        self.test = test
+
+    def setstate(self, obj): # pragma: no cover
+        self.test.fail("Connection is closed")
+
+    def register(self, obj):
+        """Does nothing."""
+
 def _len(seq):
     return len(list(seq))
 
@@ -381,6 +391,84 @@ def test_full_sweep(self):
         for oid in oids:
             self.assertTrue(cache.get(oid) is None)
 
+    def test_full_sweep_clears_weakrefs_in_interface(self, sweep_method='full_sweep'):
+        # https://github.com/zopefoundation/persistent/issues/149
+        # Sweeping the cache clears weak refs (for PyPy especially)
+        # In the real world, this shows up in the interaction with
+        # persistent objects and zope.interface/zope.component,
+        # so we use an Interface to demonstrate. This helps find issues
+        # like needing to run GC more than once, etc, because of how the
+        # object is referenced.
+        from zope.interface import Interface
+
+        gc.disable()
+        self.addCleanup(gc.enable)
+
+        jar = ClosedConnection(self)
+        cache = self._makeOne(jar, 0)
+
+        # Make a persistent object, put it in the cache as saved
+        class P(self._getRealPersistentClass()):
+            "A real persistent object that can be weak referenced"
+
+        p = P()
+        p._p_jar = jar
+        p._p_oid = b'\x01' * 8
+        cache[p._p_oid] = p
+        p._p_changed = False
+
+        # Now, take a weak reference to it from somewhere far away.
+        Interface.subscribe(p)
+
+        # Remove the original object
+        del p
+
+        # Sweep the cache.
+        getattr(cache, sweep_method)()
+
+        # Now, try to use that weak reference. If the weak reference is
+        # still around, this will raise the error about the connection
+        # being closed.
+        Interface.changed(None)
+
+    def test_incrgc_clears_weakrefs_in_interface(self):
+        self.test_full_sweep_clears_weakrefs_in_interface(sweep_method='incrgc')
+
+    def test_full_sweep_clears_weakrefs(self, sweep_method='incrgc'):
+        # like test_full_sweep_clears_weakrefs_in_interface,
+        # but directly using a weakref. This is the simplest version of the test.
+        from weakref import ref as WeakRef
+        gc.disable()
+        self.addCleanup(gc.enable)
+
+        jar = ClosedConnection(self)
+        cache = self._makeOne(jar, 0)
+
+        # Make a persistent object, put it in the cache as saved
+        class P(self._getRealPersistentClass()):
+            """A real persistent object that can be weak referenced."""
+
+        p = P()
+        p._p_jar = jar
+        p._p_oid = b'\x01' * 8
+        cache[p._p_oid] = p
+        p._p_changed = False
+
+        # Now, take a weak reference to it
+        ref = WeakRef(p)
+
+        # Remove the original object
+        del p
+
+        # Sweep the cache.
+        getattr(cache, sweep_method)()
+
+        # Now, try to use that weak reference; it should be gone.
+        p = ref()
+        self.assertIsNone(p)
+
+    def test_incrgc_clears_weakrefs(self):
+        self.test_full_sweep_clears_weakrefs(sweep_method='incrgc')
 
     def test_minimize(self):
         cache = self._makeOne()