- Removing (or adding) an event listener at the same time that the event

is being run itself, either from inside the listener or from a
concurrent thread, now raises a RuntimeError, as the collection used is
now an instance of ``colletions.deque()`` and does not support changes
while being iterated.  Previously, a plain Python list was used where
removal from inside the event itself would produce silent failures.
fixes #3163

diff --git a/doc/build/changelog/changelog_10.rst b/doc/build/changelog/changelog_10.rst
index 815de72c71a4527472cc3464e1a9bfed98e59424..fb14279ac1ac0e4d32a0b0a83a15efd939abdf3f 100644 (file)
--- a/doc/build/changelog/changelog_10.rst
+++ b/doc/build/changelog/changelog_10.rst
@@ -16,6 +16,17 @@
 .. changelog::
        :version: 1.0.0
 
+    .. change::
+        :tags: engine, bug
+        :tickets: 3163
+
+        Removing (or adding) an event listener at the same time that the event
+        is being run itself, either from inside the listener or from a
+        concurrent thread, now raises a RuntimeError, as the collection used is
+        now an instance of ``colletions.deque()`` and does not support changes
+        while being iterated.  Previously, a plain Python list was used where
+        removal from inside the event itself would produce silent failures.
+
     .. change::
         :tags: orm, feature
         :tickets: 2963

diff --git a/lib/sqlalchemy/event/api.py b/lib/sqlalchemy/event/api.py
index 270e95c9c3668870d79f10705ef6d58159678098..b3d79bcf4e5f5c4b16e5963a1bb12678636afad3 100644 (file)
--- a/lib/sqlalchemy/event/api.py
+++ b/lib/sqlalchemy/event/api.py
@@ -58,6 +58,32 @@ def listen(target, identifier, fn, *args, **kw):
     .. versionadded:: 0.9.4 Added ``once=True`` to :func:`.event.listen`
        and :func:`.event.listens_for`.
 
+    .. note::
+
+        The :func:`.listen` function cannot be called at the same time
+        that the target event is being run.   This has implications
+        for thread safety, and also means an event cannot be added
+        from inside the listener function for itself.  The list of
+        events to be run are present inside of a mutable collection
+        that can't be changed during iteration.
+
+        Event registration and removal is not intended to be a "high
+        velocity" operation; it is a configurational operation.  For
+        systems that need to quickly associate and deassociate with
+        events at high scale, use a mutable structure that is handled
+        from inside of a single listener.
+
+        .. versionchanged:: 1.0.0 - a ``collections.deque()`` object is now
+           used as the container for the list of events, which explicitly
+           disallows collection mutation while the collection is being
+           iterated.
+
+    .. seealso::
+
+        :func:`.listens_for`
+
+        :func:`.remove`
+
     """
 
     _event_key(target, identifier, fn).listen(*args, **kw)
@@ -89,6 +115,10 @@ def listens_for(target, identifier, *args, **kw):
     .. versionadded:: 0.9.4 Added ``once=True`` to :func:`.event.listen`
        and :func:`.event.listens_for`.
 
+    .. seealso::
+
+        :func:`.listen` - general description of event listening
+
     """
     def decorate(fn):
         listen(target, identifier, fn, *args, **kw)
@@ -120,6 +150,30 @@ def remove(target, identifier, fn):
 
     .. versionadded:: 0.9.0
 
+    .. note::
+
+        The :func:`.remove` function cannot be called at the same time
+        that the target event is being run.   This has implications
+        for thread safety, and also means an event cannot be removed
+        from inside the listener function for itself.  The list of
+        events to be run are present inside of a mutable collection
+        that can't be changed during iteration.
+
+        Event registration and removal is not intended to be a "high
+        velocity" operation; it is a configurational operation.  For
+        systems that need to quickly associate and deassociate with
+        events at high scale, use a mutable structure that is handled
+        from inside of a single listener.
+
+        .. versionchanged:: 1.0.0 - a ``collections.deque()`` object is now
+           used as the container for the list of events, which explicitly
+           disallows collection mutation while the collection is being
+           iterated.
+
+    .. seealso::
+
+        :func:`.listen`
+
     """
     _event_key(target, identifier, fn).remove()
 

diff --git a/lib/sqlalchemy/event/attr.py b/lib/sqlalchemy/event/attr.py
index 7641b595a857e3083034b21d14b3222a0331cb22..dba1063cfd66dd78ce741fa662624ae8267184c4 100644 (file)
--- a/lib/sqlalchemy/event/attr.py
+++ b/lib/sqlalchemy/event/attr.py
@@ -37,6 +37,7 @@ from . import registry
 from . import legacy
 from itertools import chain
 import weakref
+import collections
 
 
 class RefCollection(object):
@@ -96,8 +97,8 @@ class _DispatchDescriptor(RefCollection):
                 self.update_subclass(cls)
             else:
                 if cls not in self._clslevel:
-                    self._clslevel[cls] = []
-                self._clslevel[cls].insert(0, event_key._listen_fn)
+                    self._clslevel[cls] = collections.deque()
+                self._clslevel[cls].appendleft(event_key._listen_fn)
         registry._stored_in_collection(event_key, self)
 
     def append(self, event_key, propagate):
@@ -113,13 +114,13 @@ class _DispatchDescriptor(RefCollection):
                 self.update_subclass(cls)
             else:
                 if cls not in self._clslevel:
-                    self._clslevel[cls] = []
+                    self._clslevel[cls] = collections.deque()
                 self._clslevel[cls].append(event_key._listen_fn)
         registry._stored_in_collection(event_key, self)
 
     def update_subclass(self, target):
         if target not in self._clslevel:
-            self._clslevel[target] = []
+            self._clslevel[target] = collections.deque()
         clslevel = self._clslevel[target]
         for cls in target.__mro__[1:]:
             if cls in self._clslevel:
@@ -145,7 +146,7 @@ class _DispatchDescriptor(RefCollection):
         to_clear = set()
         for dispatcher in self._clslevel.values():
             to_clear.update(dispatcher)
-            dispatcher[:] = []
+            dispatcher.clear()
         registry._clear(self, to_clear)
 
     def for_modify(self, obj):
@@ -287,7 +288,7 @@ class _ListenerCollection(RefCollection, _CompoundListener):
         self.parent_listeners = parent._clslevel[target_cls]
         self.parent = parent
         self.name = parent.__name__
-        self.listeners = []
+        self.listeners = collections.deque()
         self.propagate = set()
 
     def for_modify(self, obj):
@@ -337,7 +338,7 @@ class _ListenerCollection(RefCollection, _CompoundListener):
     def clear(self):
         registry._clear(self, self.listeners)
         self.propagate.clear()
-        self.listeners[:] = []
+        self.listeners.clear()
 
 
 class _JoinedDispatchDescriptor(object):

diff --git a/lib/sqlalchemy/event/registry.py b/lib/sqlalchemy/event/registry.py
index a34de3cd7549bc0950402ef1dc6f7784a33b9ec1..ba2f671a3443bace856ebb9ae7a15cf20fc6f82a 100644 (file)
--- a/lib/sqlalchemy/event/registry.py
+++ b/lib/sqlalchemy/event/registry.py
@@ -243,4 +243,4 @@ class _EventKey(object):
 
     def prepend_to_list(self, owner, list_):
         _stored_in_collection(self, owner)
-        list_.insert(0, self._listen_fn)
+        list_.appendleft(self._listen_fn)

diff --git a/test/base/test_events.py b/test/base/test_events.py
index 41ccfbc353a9ad6c7bd3937b1447dc699c2a2b92..30b728cd3959b9f859543bd2c663ef9168118325 100644 (file)
--- a/test/base/test_events.py
+++ b/test/base/test_events.py
@@ -1160,3 +1160,37 @@ class RemovalTest(fixtures.TestBase):
         )
 
         event.remove(t1, "event_three", m1)
+
+    def test_no_remove_in_event(self):
+        Target = self._fixture()
+
+        t1 = Target()
+
+        def evt():
+            event.remove(t1, "event_one", evt)
+
+        event.listen(t1, "event_one", evt)
+
+        assert_raises_message(
+            Exception,
+            "deque mutated during iteration",
+            t1.dispatch.event_one
+        )
+
+    def test_no_add_in_event(self):
+        Target = self._fixture()
+
+        t1 = Target()
+
+        m1 = Mock()
+
+        def evt():
+            event.listen(t1, "event_one", m1)
+
+        event.listen(t1, "event_one", evt)
+
+        assert_raises_message(
+            Exception,
+            "deque mutated during iteration",
+            t1.dispatch.event_one
+        )