Collector 1309: ZODB.DB.DB.cacheExtremeDetail reports wrong reference count

The refcount reported for a ghost was one too small, because the code for
ghosts and non-ghosts was the same, and presumably "it's a feature" that
the code for non-ghosts deliberately lies about the true Python refcount,
in order (guessing here) to report what the count would have been if the
cPickleCache didn't exist.  But while the cPickleCache holds on to a real
reference to non-ghost objects, it does not hold a real reference to ghost
objects, so subtracting "an extra" count for all objects made it appear
that non-referenced ghosts exist in the cache (which doesn't actually
happen).

What a tangled web we weave ...

NEWS.txt

@@ -9,6 +9,8 @@ fsrecover.py couldn't work, because it referenced attributes that no
 longer existed after the MVCC changes.  Repaired that, and added new
 tests to ensure it continues working.
 
+Collector #1309:  The reference counts reported by DB.cacheExtremeDetails()
+for ghosts were one too small.
 
 
 What's new in ZODB3 3.3 alpha 3

src/ZODB/DB.py

@@ -13,7 +13,7 @@
 ##############################################################################
 """Database objects
 
-$Id: DB.py,v 1.78 2004/04/23 17:26:37 gintautasm Exp $"""
+$Id: DB.py,v 1.79 2004/05/07 19:10:59 tim_one Exp $"""
 
 import cPickle, cStringIO, sys
 from thread import allocate_lock
@@ -243,29 +243,39 @@ class DB(object):
         return detail
 
     def cacheExtremeDetail(self):
-        detail=[]
+        detail = []
         conn_no = [0]  # A mutable reference to a counter
         def f(con, detail=detail, rc=sys.getrefcount, conn_no=conn_no):
-            conn_no[0] = conn_no[0] + 1
+            conn_no[0] += 1
             cn = conn_no[0]
             for oid, ob in con._cache_items():
-                id=''
-                if hasattr(ob,'__dict__'):
-                    d=ob.__dict__
+                id = ''
+                if hasattr(ob, '__dict__'):
+                    d = ob.__dict__
                     if d.has_key('id'):
-                        id=d['id']
+                        id = d['id']
                     elif d.has_key('__name__'):
-                        id=d['__name__']
+                        id = d['__name__']
 
                 module = getattr(ob.__class__, '__module__', '')
-                module = module and '%s.' % module or ''
-
+                module = module and ('%s.' % module) or ''
+
+                # What refcount ('rc') should we return?  The intent is
+                # that we return the true Python refcount, but as if the
+                # cache didn't exist.  This routine adds 3 to the true
+                # refcount:  1 for binding to name 'ob', another because
+                # ob lives in the con._cache_items() list we're iterating
+                # over, and calling sys.getrefcount(ob) boosts ob's
+                # count by 1 too.  So the true refcount is 3 less than
+                # sys.getrefcount(ob) returns.  But, in addition to that,
+                # the cache holds an extra reference on non-ghost objects,
+                # and we also want to pretend that doesn't exist.
                 detail.append({
                     'conn_no': cn,
                     'oid': oid,
                     'id': id,
                     'klass': "%s%s" % (module, ob.__class__.__name__),
-                    'rc': rc(ob)-4,
+                    'rc': rc(ob) - 3 - (ob._p_changed is not None),
                     'state': ob._p_changed,
                     #'references': con.references(oid),
                     })

src/ZODB/tests/testCache.py

@@ -205,7 +205,7 @@ class LRUCacheTests(CacheTestBase):
         # XXX The above gc.collect call is necessary to make this test
         # pass.
         #
-        # This test then only works because the other of computations
+        # This test then only works because the order of computations
         # and object accesses in the "noodle" calls is such that the
         # persistent mapping containing the MinPO objects is
         # deactivated before the MinPO objects.
@@ -230,12 +230,15 @@ class LRUCacheTests(CacheTestBase):
                 self.assertEqual(count, CONNS)
 
         for details in self.db.cacheExtremeDetail():
-            # one dict per object.  keys:
+            # one 'details' dict per object
             if details['klass'].endswith('PersistentMapping'):
                 self.assertEqual(details['state'], None)
             else:
                 self.assert_(details['klass'].endswith('MinPO'))
                 self.assertEqual(details['state'], 0)
+            # The cache should never hold an unreferenced ghost.
+            if details['state'] is None:    # i.e., it's a ghost
+                self.assert_(details['rc'] > 0)
 
 class StubDataManager:
     def setklassstate(self, object):