[Zodb-checkins] CVS: ZODB3/ZODB - cPickleCache.c:1.85.6.5

[Tim Peters]

Update of /cvs-repository/ZODB3/ZODB
In directory cvs.zope.org:/tmp/cvs-serv22870/ZODB

Modified Files:
      Tag: Zope-2_7-branch
	cPickleCache.c 
Log Message:
Collector #1208:  Infinite loop in cPickleCache.

This fixes it, based on an approach suggested by Toby Dickenson.
The triggering condition wasn't entirely sane, so was very rare:
a persistent object with a __del__ method that referenced an
attribute of self.

scan_gc_items():  Look at objects accessed by __setattr__ and
__del__ calls no more than once.

New test checkMinimizeTerminates():  This spawns a thread that
will in fact run forever if you don't recompile cPickleCache.c.
The test suite will keep running, but checkMinimizeTerminates
will fail, and all future calls to cacheMinimize() will be
effectively ignored (so other bad things may happen as a
result).


=== ZODB3/ZODB/cPickleCache.c 1.85.6.4 => 1.85.6.5 ===
--- ZODB3/ZODB/cPickleCache.c:1.85.6.4	Tue May 18 14:31:24 2004
+++ ZODB3/ZODB/cPickleCache.c	Wed May 19 16:24:36 2004
@@ -178,20 +178,30 @@
 scan_gc_items(ccobject *self,int target)
 {
     /* This function must only be called with the ring lock held,
-       because it places a non-object placeholder in the ring.
+       because it places non-object placeholders in the ring.
     */
     cPersistentObject *object;
-    int error;
+    CPersistentRing *here;
+    CPersistentRing before_original_home;
     CPersistentRing placeholder;
-    CPersistentRing *here = self->ring_home.next;
+    int error;
     int result = -1;   /* guilty until proved innocent */
 
-    /* Scan through the ring until we either find the ring_home (i.e. start
-     * of the ring), or we've ghosted enough objects to reach the target
-     * size.
+    /* Scan the ring, from least to most recently used, ghostifying
+     * up-to-date objects, until we either find the ring_home again or
+     * or we've ghosted enough objects to reach the target size.
+     * Tricky:  __setattr__ and __del__ methods can do anything, and in
+     * particular if we ghostify an object with a __del__ method, that method
+     * can load the object again, putting it back into the MRU part of the
+     * ring.  Waiting to find ring_home again can thus cause an infinite
+     * loop (Collector #1208).  So before_original_home records the MRU
+     * position we start with, and we stop the scan when we reach that.
      */
-    while (here != &self->ring_home && self->non_ghost_count > target) {
+    insert_after(&before_original_home, self->ring_home.prev);
+    here = self->ring_home.next;   /* least recently used object */
+    while (here != &before_original_home && self->non_ghost_count > target) {
 	assert(self->ring_lock);
+	assert(here != &self->ring_home);
 
         /* At this point we know that the ring only contains nodes
 	   from persistent objects, plus our own home node. We know
@@ -203,7 +213,7 @@
         if (object->state == cPersistent_UPTODATE_STATE) {
             /* deactivate it. This is the main memory saver. */
 
-            /* Add a placeholder;  a dummy node in the ring.  We need
+            /* Add a placeholder, a dummy node in the ring.  We need
 	       to do this to mark our position in the ring.  It is
 	       possible that the PyObject_SetAttr() call below will
 	       invoke a __setattr__() hook in Python.  Also possible
@@ -232,6 +242,7 @@
     }
     result = 0;
  Done:
+    unlink_from_ring(&before_original_home);
     return result;
 }