The backport gets Fred's seal of approval:

    SF 742860: WeakKeyDictionary __delitem__ uses iterkeys

    Someone review this, please!  Final releases are getting close, Fred
    (the weakref guy) won't be around until Tuesday, and the pre-patch
    code can indeed raise spurious RuntimeErrors in the presence of
    threads or mutating comparison functions.

    See the bug report for my confusions:  I can't see any reason for why
    __delitem__ iterated over the keys.  The new one-liner implementation
    is much faster, can't raise RuntimeError, and should be better-behaved
    in all respects wrt threads.

    New tests test_weak_keyed_bad_delitem and
    test_weak_keyed_cascading_deletes fail before this patch.

Backported the tests and the patch.

diff --git a/Lib/test/test_weakref.py b/Lib/test/test_weakref.py
index 089190845fbea6f4f5440b955e9abfd77fb45c55..7a25fabc0fcbf6a744071219b087b8980a8f6159 100644 (file)
--- a/Lib/test/test_weakref.py
+++ b/Lib/test/test_weakref.py
@@ -502,6 +502,64 @@ class MappingTestCase(TestBase):
         self.assert_(len(d) == 1)
         self.assert_(d.items() == [('something else', o2)])
 
+    def test_weak_keyed_bad_delitem(self):
+        d = weakref.WeakKeyDictionary()
+        o = Object('1')
+        # An attempt to delete an object that isn't there should raise
+        # KeyError.  It didn't before 2.3.
+        self.assertRaises(KeyError, d.__delitem__, o)
+        self.assertRaises(KeyError, d.__getitem__, o)
+
+        # If a key isn't of a weakly referencable type, __getitem__ and
+        # __setitem__ raise TypeError.  __delitem__ should too.
+        self.assertRaises(TypeError, d.__delitem__,  13)
+        self.assertRaises(TypeError, d.__getitem__,  13)
+        self.assertRaises(TypeError, d.__setitem__,  13, 13)
+
+    def test_weak_keyed_cascading_deletes(self):
+        # SF bug 742860.  For some reason, before 2.3 __delitem__ iterated
+        # over the keys via self.data.iterkeys().  If things vanished from
+        # the dict during this (or got added), that caused a RuntimeError.
+
+        d = weakref.WeakKeyDictionary()
+        mutate = False
+
+        class C(object):
+            def __init__(self, i):
+                self.value = i
+            def __hash__(self):
+                return hash(self.value)
+            def __eq__(self, other):
+                if mutate:
+                    # Side effect that mutates the dict, by removing the
+                    # last strong reference to a key.
+                    del objs[-1]
+                return self.value == other.value
+
+        objs = [C(i) for i in range(4)]
+        for o in objs:
+            d[o] = o.value
+        del o   # now the only strong references to keys are in objs
+        # Find the order in which iterkeys sees the keys.
+        objs = d.keys()
+        # Reverse it, so that the iteration implementation of __delitem__
+        # has to keep looping to find the first object we delete.
+        objs.reverse()
+
+        # Turn on mutation in C.__eq__.  The first time thru the loop,
+        # under the iterkeys() business the first comparison will delete
+        # the last item iterkeys() would see, and that causes a
+        #     RuntimeError: dictionary changed size during iteration
+        # when the iterkeys() loop goes around to try comparing the next
+        # key.  After this was fixed, it just deletes the last object *our*
+        # "for o in obj" loop would have gotten to.
+        mutate = True
+        count = 0
+        for o in objs:
+            count += 1
+            del d[o]
+        self.assertEqual(len(d), 0)
+        self.assertEqual(count, 2)
 
 def test_main():
     loader = unittest.TestLoader()

diff --git a/Lib/weakref.py b/Lib/weakref.py
index bc4d0aa3bbfbf69ea54fecb26231efe9ccc5d8eb..d61c28fbd0f40473ecb76900ac3011e5920c6518 100644 (file)
--- a/Lib/weakref.py
+++ b/Lib/weakref.py
@@ -152,11 +152,7 @@ class WeakKeyDictionary(UserDict.UserDict):
         if dict is not None: self.update(dict)
 
     def __delitem__(self, key):
-        for ref in self.data.iterkeys():
-            o = ref()
-            if o == key:
-                del self.data[ref]
-                return
+        del self.data[ref(key)]
 
     def __getitem__(self, key):
         return self.data[ref(key)]