Merge lp:~divmod-dev/divmod.org/829879-pypy-finalization-semantics into lp:divmod.org

[Tristan Seligmann]

Resubmitting without the "optional-pycrypto" dependency, as this is only relevant to Mantissa.

[Laurens Van Houtven (lvh)]

LGTM, and makes my exception go away:

https://gist.github.com/lvh/9059ae79bc3ba7b10f73

Some notes:

- this doesn't appear to have any tests.
- is the API sufficiently internal remove the "has" method?

review: Needs Fixing

[Jean-Paul Calderone (exarkun)]

The change also introduces new `assert` statements. We have since learned that the way these new statements are being used is not helpful. If you want an exception to be raised, raise an exception (and document it and test it).

[Laurens Van Houtven (lvh)]

I'm not sure it really introduces any *new* ones: it just keeps the old one. (I only count three assert statements in the diff: one is just diff context; the other two are in the old and new versions of the "uncache" method -- that's the old one being kept.)

That doesn't mean changing the assertion to something else isn't a good idea, of course. That said, this is probably the minimal change that closes this ticket. Would you be okay with the assertions being changed in a different ticket? If you'd like, I will file a bug for and fix it, assuming that gets this ticket moving along :)

[Tristan Seligmann (mithrandi)]

I still need to rebase the actual branch on trunk, of course. *facepalm* I'll try to look at this soon.

[Laurens Van Houtven (lvh)]

My only remaining point is that I'm not sure about is the behavior of get. I understand this class isn't a dict, but get() mirrored dict behavior by returning a value or None. Returning a value or raising an exception is the behavior of __getitem__ :)

[lp:~divmod-dev/divmod.org/829879-pypy-finalization-semantics]

2703. By Jonathan Jacobs on 2013-07-04

Accept varargs in Divmod.UnitTest.TestCase.assertThrows.

Author: jjacobs
Reviewer: exarkun

Pass varargs to assertThrows on to the callable, matching the way Python's assertRaises works.

[Tristan Seligmann (mithrandi)]

The API of get() is not changed in this branch, unless I missed something?

[lp:~divmod-dev/divmod.org/829879-pypy-finalization-semantics]

2704. By Tristan Seligmann on 2013-07-04

Tolerate less strict finalization semantics.

2705. By Tristan Seligmann on 2013-08-02

Replace assert with raise, and repeat the "dead weakref" check logic.

2706. By Tristan Seligmann on 2013-08-02

Docstrings, comments, and better argument names.

Unmerged revisions

=== modified file 'Axiom/axiom/_fincache.py'
--- Axiom/axiom/_fincache.py	2006-04-01 19:46:29 +0000
+++ Axiom/axiom/_fincache.py	2013-07-04 16:05:30 +0000
@@ -1,4 +1,3 @@
-
 from weakref import ref
 from traceback import print_exc
 
@@ -6,11 +5,12 @@
 
 from axiom import iaxiom
 
-class CacheFault(RuntimeError):
-    """
-    A serious problem has occurred within the cache.  This error is internal
-    and should never really be trapped.
-    """
+class CacheFault(KeyError):
+    """
+    An item has fallen out of cache, but the weakref callback has not yet run.
+    """
+
+
 
 def logErrorNoMatterWhat():
     try:
@@ -27,6 +27,8 @@
             # to.  Don't bother.
             return
 
+
+
 def createCacheRemoveCallback(w, k, f):
     def remove(self):
         # Weakref callbacks cannot raise exceptions or DOOM ensues
@@ -37,12 +39,16 @@
         try:
             self = w()
             if self is not None:
-                del self.data[k]
+                try:
+                    del self.data[k]
+                except KeyError:
+                    # Already gone
+                    pass
         except:
             logErrorNoMatterWhat()
     return remove
 
-PROFILING = False
+
 
 class FinalizingCache:
     """Possibly useful for infrastructure?  This would be a nice addition (or
@@ -50,9 +56,7 @@
     """
     def __init__(self):
         self.data = {}
-        if not PROFILING:
-            # see docstring for 'has'
-            self.has = self.data.has_key
+
 
     def cache(self, key, value):
         fin = value.__finalizer__()
@@ -61,28 +65,28 @@
                 ref(self), key, fin))
         return value
 
+
     def uncache(self, key, value):
-        assert self.get(key) is value
-        del self.data[key]
-
-    def has(self, key):
-        """Does the cache have this key?
-
-        (This implementation is only used if the system is being profiled, due
-        to bugs in Python's old profiler and its interaction with weakrefs.
-        Set the module attribute PROFILING to True at startup for this.)
-        """
-        if key in self.data:
-            o = self.data[key]()
-            if o is None:
-                del self.data[key]
-                return False
-            return True
-        return False
+        """
+        Remove a key from the cache.
+
+        As a sanity check, if the specified key is present in the cache, it
+        must have the given value.
+
+        @param key: The key to remove.
+        @param value: The expected value for the key.
+        """
+        try:
+            assert self.get(key) is value
+            del self.data[key]
+        except KeyError:
+            pass
+
 
     def get(self, key):
         o = self.data[key]()
         if o is None:
+            del self.data[key]
             raise CacheFault(
                 "FinalizingCache has %r but its value is no more." % (key,))
         log.msg(interface=iaxiom.IStatEvent, stat_cache_hits=1, key=key)

=== modified file 'Axiom/axiom/store.py'
--- Axiom/axiom/store.py	2012-08-04 11:07:11 +0000
+++ Axiom/axiom/store.py	2013-07-04 16:05:30 +0000
@@ -1747,10 +1747,10 @@
             self.executeSQL(sql, insertArgs)
 
     def _loadedItem(self, itemClass, storeID, attrs):
-        if self.objectCache.has(storeID):
+        try:
             result = self.objectCache.get(storeID)
             # XXX do checks on consistency between attrs and DB object, maybe?
-        else:
+        except KeyError:
             result = itemClass.existingInStore(self, storeID, attrs)
             if not result.__legacy__:
                 self.objectCache.cache(storeID, result)
@@ -2205,8 +2205,10 @@
                     type(storeID).__name__,))
         if storeID == STORE_SELF_ID:
             return self
-        if self.objectCache.has(storeID):
+        try:
             return self.objectCache.get(storeID)
+        except KeyError:
+            pass
         log.msg(interface=iaxiom.IStatEvent, stat_cache_misses=1, key=storeID)
         results = self.querySchemaSQL(_schema.TYPEOF_QUERY, [storeID])
         assert (len(results) in [1, 0]),\

=== modified file 'Axiom/axiom/test/test_reference.py'
--- Axiom/axiom/test/test_reference.py	2009-07-06 12:35:46 +0000
+++ Axiom/axiom/test/test_reference.py	2013-07-04 16:05:30 +0000
@@ -186,7 +186,7 @@
         store = Store()
         t = nonUpgradedItem(store=store)
         self.assertEquals(t.__legacy__, True)
-        self.assertFalse(store.objectCache.has(t.storeID))
+        self.assertRaises(KeyError, store.objectCache.get, t.storeID)
         t2 = store.getItemByID(t.storeID)
         self.assertNotIdentical(t, t2)
         self.assertTrue(isinstance(t2, UpgradedItem))