Strong reference listen function wrapped by "once"

Fixed issue in event system where using the ``once=True`` flag with
dynamically generated listener functions would cause event registration of
future events to fail if those listener functions were garbage collected
after they were used, due to an assumption that a listened function is
strongly referenced.  The "once" wrapped is now modified to strongly
reference the inner function persistently, and documentation is updated
that using "once" does not imply automatic de-registration of listener
functions.

Fixes: #4794
Change-Id: I06388083d1633dcc89e8919eb1e51270f966df38

diff --git a/doc/build/changelog/unreleased_13/4794.rst b/doc/build/changelog/unreleased_13/4794.rst
new file mode 100644 (file)
index 0000000..7ba9e92
--- /dev/null
+++ b/doc/build/changelog/unreleased_13/4794.rst
@@ -0,0 +1,12 @@
+.. change::
+    :tags: bug, events
+    :tickets: 4794
+
+    Fixed issue in event system where using the ``once=True`` flag with
+    dynamically generated listener functions would cause event registration of
+    future events to fail if those listener functions were garbage collected
+    after they were used, due to an assumption that a listened function is
+    strongly referenced.  The "once" wrapped is now modified to strongly
+    reference the inner function persistently, and documentation is updated
+    that using "once" does not imply automatic de-registration of listener
+    functions.

diff --git a/lib/sqlalchemy/event/api.py b/lib/sqlalchemy/event/api.py
index e7c30901ff7fe59ac9d24f87e0a8dfcf79dd6c89..260d6e86f1d60d22f763013bf71d3794d5ede0bd 100644 (file)
--- a/lib/sqlalchemy/event/api.py
+++ b/lib/sqlalchemy/event/api.py
@@ -64,6 +64,13 @@ def listen(target, identifier, fn, *args, **kw):
     .. versionadded:: 0.9.4 Added ``once=True`` to :func:`.event.listen`
        and :func:`.event.listens_for`.
 
+    .. warning:: The ``once`` argument does not imply automatic de-registration
+       of the listener function after it has been invoked a first time; a
+       listener entry will remain associated with the target object.
+       Associating an arbitrarily high number of listeners without explictitly
+       removing them will cause memory to grow unbounded even if ``once=True``
+       is specified.
+
     .. note::
 
         The :func:`.listen` function cannot be called at the same time
@@ -124,6 +131,13 @@ def listens_for(target, identifier, *args, **kw):
     .. versionadded:: 0.9.4 Added ``once=True`` to :func:`.event.listen`
        and :func:`.event.listens_for`.
 
+    .. warning:: The ``once`` argument does not imply automatic de-registration
+       of the listener function after it has been invoked a first time; a
+       listener entry will remain associated with the target object.
+       Associating an arbitrarily high number of listeners without explictitly
+       removing them will cause memory to grow unbounded even if ``once=True``
+       is specified.
+
     .. seealso::
 
         :func:`.listen` - general description of event listening

diff --git a/lib/sqlalchemy/util/langhelpers.py b/lib/sqlalchemy/util/langhelpers.py
index d923a6aa150024a048d22b4fad6d63e2478d1ed7..0e0e3f4df57ddf77af58dc377fe23e55a1bd8867 100644 (file)
--- a/lib/sqlalchemy/util/langhelpers.py
+++ b/lib/sqlalchemy/util/langhelpers.py
@@ -1459,6 +1459,9 @@ def only_once(fn):
     once = [fn]
 
     def go(*arg, **kw):
+        # strong reference fn so that it isn't garbage collected,
+        # which interferes with the event system's expectations
+        strong_fn = fn  # noqa
         if once:
             once_fn = once.pop()
             return once_fn(*arg, **kw)

diff --git a/test/base/test_events.py b/test/base/test_events.py
index 3103b5f066b6cdc44b49ae489dfba7527d70812e..c12b3414c761a4d193ab0f4b47c904b74fbf6a73 100644 (file)
--- a/test/base/test_events.py
+++ b/test/base/test_events.py
@@ -1146,6 +1146,33 @@ class RemovalTest(fixtures.TestBase):
         eq_(m3.mock_calls, [call("x")])
         eq_(m4.mock_calls, [call("z")])
 
+    def test_once_doesnt_dereference_listener(self):
+        # test for [ticket:4794]
+
+        Target = self._fixture()
+
+        canary = Mock()
+
+        def go(target, given_id):
+            def anonymous(run_id):
+                canary(run_id, given_id)
+
+            event.listen(target, "event_one", anonymous, once=True)
+
+        t1 = Target()
+
+        assert_calls = []
+        given_ids = []
+        for given_id in range(100):
+            given_ids.append(given_id)
+            go(t1, given_id)
+            if given_id % 10 == 0:
+                t1.dispatch.event_one(given_id)
+                assert_calls.extend(call(given_id, i) for i in given_ids)
+                given_ids[:] = []
+
+        eq_(canary.mock_calls, assert_calls)
+
     def test_propagate(self):
         Target = self._fixture()