From: Mike Bayer Date: Thu, 14 Aug 2014 18:40:28 +0000 (-0400) Subject: - Removing (or adding) an event listener at the same time that the event X-Git-Tag: rel_1_0_0b1~231 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=4a4cccfee5a2eb78380e56eb9476e91658656676;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git - 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 815de72c71..fb14279ac1 100644 --- 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 270e95c9c3..b3d79bcf4e 100644 --- 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 7641b595a8..dba1063cfd 100644 --- 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 a34de3cd75..ba2f671a34 100644 --- 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 41ccfbc353..30b728cd39 100644 --- 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 + )