Fixed removal of signal receivers in Python 3.4

Make use of `weakref.finalize` and `weakref.WeakMethod` on python 3.4.
Simplified the removal of receivers, the old function looked overly
complicated.

Many thanks go to Antoine Pitrou for helping me to debug and explain all
the failures I ran into while writing that patch.

django/dispatch/dispatcher.py

@@ -1,11 +1,13 @@
-import weakref
+import sys
 import threading
+import weakref
 
-from django.dispatch import saferef
 from django.utils.six.moves import xrange
 
-
-WEAKREF_TYPES = (weakref.ReferenceType, saferef.BoundMethodWeakref)
+if sys.version_info < (3,4):
+    from .weakref_backports import WeakMethod
+else:
+    from weakref import WeakMethod
 
 
 def _make_id(target):
@@ -57,9 +59,7 @@ class Signal(object):
                 A function or an instance method which is to receive signals.
                 Receivers must be hashable objects.
 
-                If weak is True, then receiver must be weak-referencable (more
-                precisely saferef.safeRef() must be able to create a reference
-                to the receiver).
+                If weak is True, then receiver must be weak-referencable.
 
                 Receivers must be able to accept keyword arguments.
 
@@ -105,20 +105,33 @@ class Signal(object):
                 assert argspec[2] is not None, \
                     "Signal receivers must accept keyword arguments (**kwargs)."
 
+        receiver_id = _make_id(receiver)
         if dispatch_uid:
             lookup_key = (dispatch_uid, _make_id(sender))
         else:
-            lookup_key = (_make_id(receiver), _make_id(sender))
+            lookup_key = (receiver_id, _make_id(sender))
 
         if weak:
-            receiver = saferef.safeRef(receiver, onDelete=self._remove_receiver)
+            ref = weakref.ref
+            original_receiver = receiver
+            # Check for bound methods
+            if hasattr(receiver, '__self__') and hasattr(receiver, '__func__'):
+                ref = WeakMethod
+                original_receiver = original_receiver.__self__
+            if sys.version_info >= (3, 4):
+                receiver = ref(receiver)
+                weakref.finalize(original_receiver, self._remove_receiver, receiver_id=receiver_id)
+            else:
+                receiver = ref(receiver, self._remove_receiver)
+                # Use the id of the weakref, since that's what passed to the weakref callback!
+                receiver_id = _make_id(receiver)
 
         with self.lock:
-            for r_key, _ in self.receivers:
+            for r_key, _, _ in self.receivers:
                 if r_key == lookup_key:
                     break
             else:
-                self.receivers.append((lookup_key, receiver))
+                self.receivers.append((lookup_key, receiver, receiver_id))
             self.sender_receivers_cache.clear()
 
     def disconnect(self, receiver=None, sender=None, weak=True, dispatch_uid=None):
@@ -150,7 +163,7 @@ class Signal(object):
 
         with self.lock:
             for index in xrange(len(self.receivers)):
-                (r_key, _) = self.receivers[index]
+                (r_key, _, _) = self.receivers[index]
                 if r_key == lookup_key:
                     del self.receivers[index]
                     break
@@ -242,7 +255,7 @@ class Signal(object):
             with self.lock:
                 senderkey = _make_id(sender)
                 receivers = []
-                for (receiverkey, r_senderkey), receiver in self.receivers:
+                for (receiverkey, r_senderkey), receiver, _ in self.receivers:
                     if r_senderkey == NONE_ID or r_senderkey == senderkey:
                         receivers.append(receiver)
                 if self.use_caching:
@@ -253,7 +266,7 @@ class Signal(object):
                         self.sender_receivers_cache[sender] = receivers
         non_weak_receivers = []
         for receiver in receivers:
-            if isinstance(receiver, WEAKREF_TYPES):
+            if isinstance(receiver, weakref.ReferenceType):
                 # Dereference the weak reference.
                 receiver = receiver()
                 if receiver is not None:
@@ -262,23 +275,19 @@ class Signal(object):
                 non_weak_receivers.append(receiver)
         return non_weak_receivers
 
-    def _remove_receiver(self, receiver):
+    def _remove_receiver(self, receiver=None, receiver_id=None, _make_id=_make_id):
         """
         Remove dead receivers from connections.
-        """
 
+        `receiver_id` is used by python 3.4 and up. `receiver` is used in older
+        versions and is the weakref to the receiver (if the connection was defined
+        as `weak`). We also need to pass on `_make_id` since the original reference
+        will be None during module shutdown.
+        """
         with self.lock:
-            to_remove = []
-            for key, connected_receiver in self.receivers:
-                if connected_receiver == receiver:
-                    to_remove.append(key)
-            for key in to_remove:
-                last_idx = len(self.receivers) - 1
-                # enumerate in reverse order so that indexes are valid even
-                # after we delete some items
-                for idx, (r_key, _) in enumerate(reversed(self.receivers)):
-                    if r_key == key:
-                        del self.receivers[last_idx - idx]
+            if receiver is not None:
+                receiver_id = _make_id(receiver)
+            self.receivers[:] = [val for val in self.receivers if val[2] != receiver_id]
             self.sender_receivers_cache.clear()
 
 

django/dispatch/saferef.py

@@ -1,261 +0,0 @@
-"""
-"Safe weakrefs", originally from pyDispatcher.
-
-Provides a way to safely weakref any function, including bound methods (which
-aren't handled by the core weakref module).
-"""
-
-import traceback
-import weakref
-
-
-def safeRef(target, onDelete=None):
-    """Return a *safe* weak reference to a callable target
-
-    target -- the object to be weakly referenced, if it's a
-        bound method reference, will create a BoundMethodWeakref,
-        otherwise creates a simple weakref.
-    onDelete -- if provided, will have a hard reference stored
-        to the callable to be called after the safe reference
-        goes out of scope with the reference object, (either a
-        weakref or a BoundMethodWeakref) as argument.
-    """
-    if hasattr(target, '__self__'):
-        if target.__self__ is not None:
-            # Turn a bound method into a BoundMethodWeakref instance.
-            # Keep track of these instances for lookup by disconnect().
-            assert hasattr(target, '__func__'), """safeRef target %r has __self__, but no __func__, don't know how to create reference""" % (target,)
-            reference = get_bound_method_weakref(
-                target=target,
-                onDelete=onDelete
-            )
-            return reference
-    if callable(onDelete):
-        return weakref.ref(target, onDelete)
-    else:
-        return weakref.ref(target)
-
-
-class BoundMethodWeakref(object):
-    """'Safe' and reusable weak references to instance methods
-
-    BoundMethodWeakref objects provide a mechanism for
-    referencing a bound method without requiring that the
-    method object itself (which is normally a transient
-    object) is kept alive.  Instead, the BoundMethodWeakref
-    object keeps weak references to both the object and the
-    function which together define the instance method.
-
-    Attributes:
-        key -- the identity key for the reference, calculated
-            by the class's calculateKey method applied to the
-            target instance method
-        deletionMethods -- sequence of callable objects taking
-            single argument, a reference to this object which
-            will be called when *either* the target object or
-            target function is garbage collected (i.e. when
-            this object becomes invalid).  These are specified
-            as the onDelete parameters of safeRef calls.
-        weakSelf -- weak reference to the target object
-        weakFunc -- weak reference to the target function
-
-    Class Attributes:
-        _allInstances -- class attribute pointing to all live
-            BoundMethodWeakref objects indexed by the class's
-            calculateKey(target) method applied to the target
-            objects.  This weak value dictionary is used to
-            short-circuit creation so that multiple references
-            to the same (object, function) pair produce the
-            same BoundMethodWeakref instance.
-
-    """
-
-    _allInstances = weakref.WeakValueDictionary()
-
-    def __new__(cls, target, onDelete=None, *arguments, **named):
-        """Create new instance or return current instance
-
-        Basically this method of construction allows us to
-        short-circuit creation of references to already-
-        referenced instance methods.  The key corresponding
-        to the target is calculated, and if there is already
-        an existing reference, that is returned, with its
-        deletionMethods attribute updated.  Otherwise the
-        new instance is created and registered in the table
-        of already-referenced methods.
-        """
-        key = cls.calculateKey(target)
-        current = cls._allInstances.get(key)
-        if current is not None:
-            current.deletionMethods.append(onDelete)
-            return current
-        else:
-            base = super(BoundMethodWeakref, cls).__new__(cls)
-            cls._allInstances[key] = base
-            base.__init__(target, onDelete, *arguments, **named)
-            return base
-
-    def __init__(self, target, onDelete=None):
-        """Return a weak-reference-like instance for a bound method
-
-        target -- the instance-method target for the weak
-            reference, must have __self__ and __func__ attributes
-            and be reconstructable via:
-                target.__func__.__get__( target.__self__ )
-            which is true of built-in instance methods.
-        onDelete -- optional callback which will be called
-            when this weak reference ceases to be valid
-            (i.e. either the object or the function is garbage
-            collected).  Should take a single argument,
-            which will be passed a pointer to this object.
-        """
-        def remove(weak, self=self):
-            """Set self.isDead to true when method or instance is destroyed"""
-            methods = self.deletionMethods[:]
-            del self.deletionMethods[:]
-            try:
-                del self.__class__._allInstances[self.key]
-            except KeyError:
-                pass
-            for function in methods:
-                try:
-                    if callable(function):
-                        function(self)
-                except Exception as e:
-                    try:
-                        traceback.print_exc()
-                    except AttributeError:
-                        print('Exception during saferef %s cleanup function %s: %s' % (
-                            self, function, e)
-                        )
-        self.deletionMethods = [onDelete]
-        self.key = self.calculateKey(target)
-        self.weakSelf = weakref.ref(target.__self__, remove)
-        self.weakFunc = weakref.ref(target.__func__, remove)
-        self.selfName = str(target.__self__)
-        self.funcName = str(target.__func__.__name__)
-
-    @classmethod
-    def calculateKey(cls, target):
-        """Calculate the reference key for this reference
-
-        Currently this is a two-tuple of the id()'s of the
-        target object and the target function respectively.
-        """
-        return (id(target.__self__), id(target.__func__))
-
-    def __str__(self):
-        """Give a friendly representation of the object"""
-        return """%s( %s.%s )""" % (
-            self.__class__.__name__,
-            self.selfName,
-            self.funcName,
-        )
-
-    __repr__ = __str__
-
-    def __hash__(self):
-        return hash(self.key)
-
-    def __bool__(self):
-        """Whether we are still a valid reference"""
-        return self() is not None
-
-    def __nonzero__(self):      # Python 2 compatibility
-        return type(self).__bool__(self)
-
-    def __eq__(self, other):
-        """Compare with another reference"""
-        if not isinstance(other, self.__class__):
-            return self.__class__ == type(other)
-        return self.key == other.key
-
-    def __call__(self):
-        """Return a strong reference to the bound method
-
-        If the target cannot be retrieved, then will
-        return None, otherwise returns a bound instance
-        method for our object and function.
-
-        Note:
-            You may call this method any number of times,
-            as it does not invalidate the reference.
-        """
-        target = self.weakSelf()
-        if target is not None:
-            function = self.weakFunc()
-            if function is not None:
-                return function.__get__(target)
-        return None
-
-
-class BoundNonDescriptorMethodWeakref(BoundMethodWeakref):
-    """A specialized BoundMethodWeakref, for platforms where instance methods
-    are not descriptors.
-
-    It assumes that the function name and the target attribute name are the
-    same, instead of assuming that the function is a descriptor. This approach
-    is equally fast, but not 100% reliable because functions can be stored on an
-    attribute named differenty than the function's name such as in:
-
-    class A: pass
-    def foo(self): return "foo"
-    A.bar = foo
-
-    But this shouldn't be a common use case. So, on platforms where methods
-    aren't descriptors (such as Jython) this implementation has the advantage
-    of working in the most cases.
-    """
-    def __init__(self, target, onDelete=None):
-        """Return a weak-reference-like instance for a bound method
-
-        target -- the instance-method target for the weak
-            reference, must have __self__ and __func__ attributes
-            and be reconstructable via:
-                target.__func__.__get__( target.__self__ )
-            which is true of built-in instance methods.
-        onDelete -- optional callback which will be called
-            when this weak reference ceases to be valid
-            (i.e. either the object or the function is garbage
-            collected).  Should take a single argument,
-            which will be passed a pointer to this object.
-        """
-        assert getattr(target.__self__, target.__name__) == target, \
-            ("method %s isn't available as the attribute %s of %s" %
-                (target, target.__name__, target.__self__))
-        super(BoundNonDescriptorMethodWeakref, self).__init__(target, onDelete)
-
-    def __call__(self):
-        """Return a strong reference to the bound method
-
-        If the target cannot be retrieved, then will
-        return None, otherwise returns a bound instance
-        method for our object and function.
-
-        Note:
-            You may call this method any number of times,
-            as it does not invalidate the reference.
-        """
-        target = self.weakSelf()
-        if target is not None:
-            function = self.weakFunc()
-            if function is not None:
-                # Using partial() would be another option, but it erases the
-                # "signature" of the function. That is, after a function is
-                # curried, the inspect module can't be used to determine how
-                # many arguments the function expects, nor what keyword
-                # arguments it supports, and pydispatcher needs this
-                # information.
-                return getattr(target, function.__name__)
-        return None
-
-
-def get_bound_method_weakref(target, onDelete):
-    """Instantiates the appropiate BoundMethodWeakRef, depending on the details of
-    the underlying class method implementation"""
-    if hasattr(target, '__get__'):
-        # target method is a descriptor, so the default implementation works:
-        return BoundMethodWeakref(target=target, onDelete=onDelete)
-    else:
-        # no luck, use the alternative implementation:
-        return BoundNonDescriptorMethodWeakref(target=target, onDelete=onDelete)

django/dispatch/weakref_backports.py

@@ -0,0 +1,62 @@
+"""
+weakref_backports is a partial backport of the weakref module for python
+versions below 3.4.
+
+TODO: LICENSE!
+"""
+from weakref import ref
+
+
+class WeakMethod(ref):
+    """
+    A custom `weakref.ref` subclass which simulates a weak reference to
+    a bound method, working around the lifetime problem of bound methods.
+    """
+
+    __slots__ = "_func_ref", "_meth_type", "_alive", "__weakref__"
+
+    def __new__(cls, meth, callback=None):
+        try:
+            obj = meth.__self__
+            func = meth.__func__
+        except AttributeError:
+            raise TypeError("argument should be a bound method, not {}"
+                            .format(type(meth)))
+        def _cb(arg):
+            # The self-weakref trick is needed to avoid creating a reference
+            # cycle.
+            self = self_wr()
+            if self._alive:
+                self._alive = False
+                if callback is not None:
+                    callback(self)
+        self = ref.__new__(cls, obj, _cb)
+        self._func_ref = ref(func, _cb)
+        self._meth_type = type(meth)
+        self._alive = True
+        self_wr = ref(self)
+        return self
+
+    def __call__(self):
+        obj = super(WeakMethod, self).__call__()
+        func = self._func_ref()
+        if obj is None or func is None:
+            return None
+        return self._meth_type(func, obj)
+
+    def __eq__(self, other):
+        if isinstance(other, WeakMethod):
+            if not self._alive or not other._alive:
+                return self is other
+            return ref.__eq__(self, other) and self._func_ref == other._func_ref
+        return False
+
+    def __ne__(self, other):
+        if isinstance(other, WeakMethod):
+            if not self._alive or not other._alive:
+                return self is not other
+            return ref.__ne__(self, other) or self._func_ref != other._func_ref
+        return True
+
+    __hash__ = ref.__hash__
+

tests/dispatch/tests/test_saferef.py

@@ -1,76 +0,0 @@
-import unittest
-
-from django.dispatch.saferef import safeRef
-from django.utils.six.moves import xrange
-
-
-class Test1(object):
-    def x(self):
-        pass
-
-
-def test2(obj):
-    pass
-
-
-class Test2(object):
-    def __call__(self, obj):
-        pass
-
-
-class SaferefTests(unittest.TestCase):
-    def setUp(self):
-        ts = []
-        ss = []
-        for x in xrange(5000):
-            t = Test1()
-            ts.append(t)
-            s = safeRef(t.x, self._closure)
-            ss.append(s)
-        ts.append(test2)
-        ss.append(safeRef(test2, self._closure))
-        for x in xrange(30):
-            t = Test2()
-            ts.append(t)
-            s = safeRef(t, self._closure)
-            ss.append(s)
-        self.ts = ts
-        self.ss = ss
-        self.closureCount = 0
-
-    def tearDown(self):
-        del self.ts
-        del self.ss
-
-    def testIn(self):
-        """Test the "in" operator for safe references (cmp)"""
-        for t in self.ts[:50]:
-            self.assertTrue(safeRef(t.x) in self.ss)
-
-    def testValid(self):
-        """Test that the references are valid (return instance methods)"""
-        for s in self.ss:
-            self.assertTrue(s())
-
-    def testShortCircuit(self):
-        """Test that creation short-circuits to reuse existing references"""
-        sd = {}
-        for s in self.ss:
-            sd[s] = 1
-        for t in self.ts:
-            if hasattr(t, 'x'):
-                self.assertTrue(safeRef(t.x) in sd)
-            else:
-                self.assertTrue(safeRef(t) in sd)
-
-    def testRepresentation(self):
-        """Test that the reference object's representation works
-
-        XXX Doesn't currently check the results, just that no error
-            is raised
-        """
-        repr(self.ss[-1])
-
-    def _closure(self, ref):
-        """Dumb utility mechanism to increment deletion counter"""
-        self.closureCount += 1