Deprecate safeweakref and replace its uses

envisage/extension_registry.py

@@ -11,21 +11,55 @@
 
 # Standard library imports.
 import logging
+import types
+import weakref
 
 # Enthought library imports.
 from traits.api import Dict, HasTraits, provides
 
 # Local imports.
 from .extension_point_changed_event import ExtensionPointChangedEvent
 from .i_extension_registry import IExtensionRegistry
-from . import safeweakref
 from .unknown_extension_point import UnknownExtensionPoint
 
 
 # Logging.
 logger = logging.getLogger(__name__)
 
 
+def _saferef(listener):
+    """
+    Weak reference for a (possibly bound method) listener.
+
+    Returns a weakref.WeakMethod reference for bound methods,
+    and a regular weakref.ref otherwise.
+
+    This means that for example ``_saferef(myobj.mymethod)``
+    returns a reference whose lifetime is connected to the
+    lifetime of the object ``myobj``, rather than the lifetime
+    of the temporary method ``myobj.mymethod``.
+
+    Parameters
+    ----------
+    listener : callable
+        Listener to return a weak reference for. This can be
+        either a plain function, a bound method, or some other
+        form of callable.
+
+    Returns
+    -------
+    weakref.ref
+        A weak reference to the listener. This will be a ``weakref.WeakMethod``
+        object if the listener is an instance of ``types.MethodType``, and a
+        plain ``weakref.ref`` otherwise.
+
+    """
+    if isinstance(listener, types.MethodType):
+        return weakref.WeakMethod(listener)
+    else:
+        return weakref.ref(listener)
+
+
 @provides(IExtensionRegistry)
 class ExtensionRegistry(HasTraits):
     """ A base class for extension registry implementation. """
@@ -61,7 +95,7 @@ def add_extension_point_listener(self, listener, extension_point_id=None):
         """ Add a listener for extensions being added or removed. """
 
         listeners = self._listeners.setdefault(extension_point_id, [])
-        listeners.append(safeweakref.ref(listener))
+        listeners.append(_saferef(listener))
 
         return
 
@@ -94,7 +128,7 @@ def remove_extension_point_listener(
         """ Remove a listener for extensions being added or removed. """
 
         listeners = self._listeners.setdefault(extension_point_id, [])
-        listeners.remove(safeweakref.ref(listener))
+        listeners.remove(_saferef(listener))
 
         return
 

envisage/safeweakref.py

@@ -19,99 +19,41 @@
 """
 
 # Standard library imports.
-import sys
+import warnings
 import weakref
 
-# Because this module is intended as a drop-in replacement for weakref, we
-# import everything from that module here (so the user can do things like
-# "import safeweakref as weakref" etc).
 
-from weakref import *  # noqa: F403
-
-__all__ = weakref.__all__
-
-if hasattr(weakref, "WeakMethod"):
-    pass
-else:
-    # Backport WeakMethod from Python 3
-    class WeakMethod(weakref.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 = weakref.ref.__new__(cls, obj, _cb)
-            self._func_ref = weakref.ref(func, _cb)
-            self._meth_type = type(meth)
-            self._alive = True
-            self_wr = weakref.ref(self)
-            return self
-
-        def __call__(self):
-            obj = weakref.ref.__call__(self)
-            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 (
-                    weakref.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 (
-                    weakref.ref.__ne__(self, other)
-                    or self._func_ref != other._func_ref
-                )
-            return True
+class ref(object):
+    """An implementation of weak references that works for bound methods and
+    caches them.
 
-        __hash__ = weakref.ref.__hash__
+    If ``object`` is a bound method, returns a ``weakref.WeakMethod`` for that
+    method. This ensures that the method is kept alive for the lifetime of the
+    object that it's bound to.
 
+    For any other ``object``, a normal ``weakref.ref`` is returned.
 
-class ref(object):
-    """ An implementation of weak references that works for bound methods and \
-    caches them. """
+    .. deprecated:: 5.0.0
 
+    """
     _cache = weakref.WeakKeyDictionary()
 
     def __new__(cls, obj, callback=None):
+        warnings.warn(
+            message=(
+                "safeweakref.ref is deprecated, and will be removed in a "
+                "future version of Envisage"
+            ),
+            category=DeprecationWarning,
+            stacklevel=2,
+        )
+
         if getattr(obj, "__self__", None) is not None:  # Bound method
             # Caching
             func_cache = cls._cache.setdefault(obj.__self__, {})
             self = func_cache.get(obj.__func__)
             if self is None:
-                self = WeakMethod(obj, callback)
+                self = weakref.WeakMethod(obj, callback)
                 func_cache[obj.__func__] = self
             return self
         else:

envisage/tests/test_extension_registry.py

@@ -9,6 +9,7 @@
 """ Tests for the base extension registry. """
 
 # Standard library imports.
+import contextlib
 import unittest
 
 # Enthought library imports.
@@ -149,3 +150,133 @@ def _create_extension_point(self, id, trait_type=List, desc=""):
         """ Create an extension point. """
 
         return ExtensionPoint(id=id, trait_type=trait_type, desc=desc)
+
+
+def make_function_listener(events):
+    """
+    Return a simple non-method extension point listener.
+
+    The listener appends events to the ``events`` list.
+    """
+    def listener(registry, event):
+        events.append(event)
+
+    return listener
+
+
+class ListensToExtensionPoint:
+    """
+    Class with a method that can be used as an extension point listener.
+    """
+    def __init__(self, events):
+        self.events = events
+
+    def listener(self, registry, event):
+        self.events.append(event)
+
+
+class ExtensionPointListenerLifetimeTestCase(unittest.TestCase):
+    def setUp(self):
+        # We do all of the testing via the application to make sure it offers
+        # the same interface!
+        registry = Application(extension_registry=ExtensionRegistry())
+        extension_point = ExtensionPoint(id="my.ep", trait_type=List())
+        registry.add_extension_point(extension_point)
+        self.registry = registry
+
+        # A place to record events that listeners receive.
+        self.events = []
+
+    def test_add_nonmethod_listener(self):
+        listener = make_function_listener(self.events)
+        self.registry.add_extension_point_listener(listener, "my.ep")
+
+        with self.assertAppendsTo(self.events):
+            self.registry.set_extensions("my.ep", [1, 2, 3])
+
+    def test_remove_nonmethod_listener(self):
+        listener = make_function_listener(self.events)
+
+        self.registry.add_extension_point_listener(listener, "my.ep")
+        self.registry.remove_extension_point_listener(listener, "my.ep")
+
+        with self.assertDoesNotModify(self.events):
+            self.registry.set_extensions("my.ep", [4, 5, 6, 7])
+
+    def test_nonmethod_listener_lifetime(self):
+        listener = make_function_listener(self.events)
+        self.registry.add_extension_point_listener(listener, "my.ep")
+
+        # The listener should not kept alive by the registry.
+        del listener
+
+        with self.assertDoesNotModify(self.events):
+            self.registry.set_extensions("my.ep", [4, 5, 6, 7])
+
+    def test_add_method_listener(self):
+        obj = ListensToExtensionPoint(self.events)
+        self.registry.add_extension_point_listener(obj.listener, "my.ep")
+
+        # At this point, the bound method 'obj.listener' no longer
+        # exists; it's already been garbage collected. Nevertheless, the
+        # listener should still fire.
+        with self.assertAppendsTo(self.events):
+            self.registry.set_extensions("my.ep", [1, 2, 3])
+
+    def test_remove_method_listener(self):
+        obj = ListensToExtensionPoint(self.events)
+        # The two occurences of `obj.listener` below refer to different
+        # objects. Nevertheless, they _compare_ equal, so the removal
+        # should still be effective.
+        self.registry.add_extension_point_listener(obj.listener, "my.ep")
+        self.registry.remove_extension_point_listener(obj.listener, "my.ep")
+
+        with self.assertDoesNotModify(self.events):
+            self.registry.set_extensions("my.ep", [1, 2, 3])
+
+    def test_method_listener_lifetime(self):
+        obj = ListensToExtensionPoint(self.events)
+        self.registry.add_extension_point_listener(obj.listener, "my.ep")
+
+        # Removing the last reference to the object should deactivate
+        # the listener.
+        del obj
+
+        with self.assertDoesNotModify(self.events):
+            self.registry.set_extensions("my.ep", [1, 2, 3])
+
+    # Helper assertions #######################################################
+
+    @contextlib.contextmanager
+    def assertAppendsTo(self, some_list):
+        """
+        Assert that exactly one element is appended to a list.
+
+        Return a context manager that checks that the code in the corresponding
+        with block appends exactly one element to the given list.
+        """
+        old_length = len(some_list)
+        yield
+        new_length = len(some_list)
+        diff = new_length - old_length
+        self.assertEqual(
+            diff, 1,
+            msg="Expected exactly one new element; got {}".format(diff),
+        )
+
+    @contextlib.contextmanager
+    def assertDoesNotModify(self, some_list):
+        """
+        Assert that a list is unchanged.
+
+        Return a context manager that checks that the code in the corresponding
+        with block does not modify the length of the given list.
+        """
+        old_length = len(some_list)
+        yield
+        new_length = len(some_list)
+        diff = new_length - old_length
+        self.assertEqual(
+            diff, 0,
+            msg="Expected no new elements; got {}".format(diff),
+        )