Fixes for circular dependencies preventing Entity destruction, and Listener re/un-assignment (#129)

* Don't rely on the _entities dictionary in the Entity finalizer.

In the presence of circular references the object being finalized may
well have already been removed from the WeakValueDictionary that makes
up the _entities store. See: https://gist.github.com/willstott101/8544a7c966b0788ff4f93da410e9fea7

* Support un-assigning listeners in Entity, and fix bug with reassignment.

* Add test for listener reassignment corner-cases

Author: willstott101

cyclonedds/core.py

@@ -187,10 +187,10 @@ def __init__(self, ref: int, listener: "Listener" = None) -> None:
         self._listener = listener
 
     def __del__(self) -> None:
-        if not hasattr(self, "_ref") or self._ref not in self._entities:
+        if not hasattr(self, "_ref"):
             return
 
-        del self._entities[self._ref]
+        self._entities.pop(self._ref, None)
         self._delete(self._ref)
 
     def get_subscriber(self) -> Optional["cyclonedds.sub.Subscriber"]:
@@ -474,23 +474,33 @@ def get_listener(self) -> "Listener":
         """
         return self._listener.copy() if self._listener else Listener()
 
-    def set_listener(self, listener: "Listener") -> None:
+    def set_listener(self, listener: Optional["Listener"]) -> None:
         """Update the listener for this object. If a listener already exist for this object only the fields you explicitly
-        have set on your new listener are overwritten.
+        have set on your new listener are overwritten. Passing None will remove this entity's Listener.
+
+        Future changes to the passed Listener object will not affect the Listener associated with this Entity.
 
         Parameters
         ----------
-        listener : Listener
-            The listener object to use.
+        listener :
+            The listener object to use, or None to remove the current listener from this Entity.
 
         Raises
         ------
         DDSException
         """
-        if self._listener != listener:
-            listener.copy_to(self._listener)
+        if listener is not None:
+            if self._listener is not None:
+                if self._listener != listener:
+                    listener.copy_to(self._listener)
+            else:
+                self._listener = listener.copy()
+            ref = self._listener._ref
+        else:
+            ref = None
+            self._listener = None
 
-        ret = self._set_listener(self._ref, listener._ref)
+        ret = self._set_listener(self._ref, ref)
         if ret == 0:
             return
         raise DDSException(ret, f"Occurred when setting the Listener for {repr(self)}")

tests/test_entity.py

@@ -178,6 +178,46 @@ def test_set_listener():
     dp.set_listener(Listener())
 
 
+def test_listener_reassignment(manual_setup, hitpoint_factory):
+    class L(Listener):
+        def __init__(self):
+            super().__init__()
+            self.hitpoint = hitpoint_factory()
+
+        def on_data_available(self, _):
+            self.hitpoint.hit()
+
+    l_1 = L()
+
+    # Check we can recieve events
+    dr = manual_setup.dr(listener=l_1)
+    manual_setup.dw().write(manual_setup.msg)
+
+    assert l_1.hitpoint.was_hit()
+    l_1.hitpoint.hp.clear()
+
+    # Check we stop recieving events after setting None
+    dr.set_listener(None)
+    manual_setup.dw().write(manual_setup.msg)
+
+    assert l_1.hitpoint.was_not_hit()
+
+    # Check we can recieve events after re-assigning the same handler from None
+    dr.set_listener(l_1)
+    manual_setup.dw().write(manual_setup.msg)
+
+    assert l_1.hitpoint.was_hit()
+    l_1.hitpoint.hp.clear()
+
+    # Check we can recieve events from a new handler that replaces the old one
+    l_2 = L()
+    dr.set_listener(l_2)
+    manual_setup.dw().write(manual_setup.msg)
+
+    assert l_1.hitpoint.was_not_hit()
+    assert l_2.hitpoint.was_hit()
+
+
 def test_retain_listener():
     m = lambda x, y: 0
     l = Listener(on_data_available=m)