Fixed #21952 -- signals deadlock due to locking + weakref interaction

django/dispatch/dispatcher.py

@@ -48,6 +48,7 @@ class Signal(object):
         # 'sender_receivers_cache'. The cache is cleaned when .connect() or
         # .disconnect() is called and populated on send().
         self.sender_receivers_cache = weakref.WeakKeyDictionary() if use_caching else {}
+        self._dead_receivers = False
 
     def connect(self, receiver, sender=None, weak=True, dispatch_uid=None):
         """
@@ -127,6 +128,7 @@ class Signal(object):
                 receiver_id = _make_id(receiver)
 
         with self.lock:
+            self._clear_dead_receivers()
             for r_key, _, _ in self.receivers:
                 if r_key == lookup_key:
                     break
@@ -162,6 +164,7 @@ class Signal(object):
             lookup_key = (_make_id(receiver), _make_id(sender))
 
         with self.lock:
+            self._clear_dead_receivers()
             for index in xrange(len(self.receivers)):
                 (r_key, _, _) = self.receivers[index]
                 if r_key == lookup_key:
@@ -237,6 +240,17 @@ class Signal(object):
                 responses.append((receiver, response))
         return responses
 
+    def _clear_dead_receivers(self):
+        # Note: caller is assumed to hold self.lock.
+        if self._dead_receivers:
+            self._dead_receivers = False
+            new_receivers = []
+            for r in self.receivers:
+                if isinstance(r[1], weakref.ReferenceType) and r[1]() is None:
+                    continue
+                new_receivers.append(r)
+            self.receivers = new_receivers
+
     def _live_receivers(self, sender):
         """
         Filter sequence of receivers to get resolved, live receivers.
@@ -245,7 +259,7 @@ class Signal(object):
         live receivers.
         """
         receivers = None
-        if self.use_caching:
+        if self.use_caching and not self._dead_receivers:
             receivers = self.sender_receivers_cache.get(sender)
             # We could end up here with NO_RECEIVERS even if we do check this case in
             # .send() prior to calling _live_receivers() due to concurrent .send() call.
@@ -253,6 +267,7 @@ class Signal(object):
                 return []
         if receivers is None:
             with self.lock:
+                self._clear_dead_receivers()
                 senderkey = _make_id(sender)
                 receivers = []
                 for (receiverkey, r_senderkey), receiver, _ in self.receivers:
@@ -276,19 +291,13 @@ class Signal(object):
         return non_weak_receivers
 
     def _remove_receiver(self, receiver=None, receiver_id=None, _make_id=_make_id):
-        """
+        # Mark that the self.receivers list has dead weakrefs. If so, we will
-        Remove dead receivers from connections.
+        # clean those up in connect, disconnect and _live_receivers while
-
+        # holding self.lock. Note that doing the cleanup here isn't a good
-        `receiver_id` is used by python 3.4 and up. `receiver` is used in older
+        # idea, _remove_receiver() will be called as side effect of garbage
-        versions and is the weakref to the receiver (if the connection was defined
+        # collection, and so the call can happen while we are already holding
-        as `weak`). We also need to pass on `_make_id` since the original reference
+        # self.lock.
-        will be None during module shutdown.
+        self._dead_receivers = True
-        """
-        with self.lock:
-            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()
 
 
 def receiver(signal, **kwargs):

tests/dispatch/tests/test_dispatcher.py

@@ -46,11 +46,12 @@ class DispatcherTests(unittest.TestCase):
 
     def _testIsClean(self, signal):
         """Assert that everything has been cleaned up automatically"""
+        # Note that dead weakref cleanup happens as side effect of using
+        # the signal's receivers through the signals API. So, first do a
+        # call to an API method to force cleanup.
+        self.assertFalse(signal.has_listeners())
         self.assertEqual(signal.receivers, [])
 
-        # force cleanup just in case
-        signal.receivers = []
-
     def testExact(self):
         a_signal.connect(receiver_1_arg, sender=self)
         expected = [(receiver_1_arg, "test")]