From: Mike Bayer Date: Fri, 3 Jan 2025 17:19:27 +0000 (-0500) Subject: guard against KeyError on subclass removal X-Git-Tag: rel_2_0_37~4 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=3304bc58a6d6d79db7c87fe4fe569a455a35a3b0;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git guard against KeyError on subclass removal Fixed issue in event system which prevented an event listener from being attached and detached from multiple class-like objects, namely the :class:`.sessionmaker` or :class:`.scoped_session` targets that assign to :class:`.Session` subclasses. Fixes: #12216 Change-Id: I3d8969fe604adbc23add07a13741938c7f4fc8ca (cherry picked from commit e4f0afe06baa5d9b57d5b8cfe2647b943f2145e6) --- diff --git a/doc/build/changelog/unreleased_20/12216.rst b/doc/build/changelog/unreleased_20/12216.rst new file mode 100644 index 0000000000..a412673335 --- /dev/null +++ b/doc/build/changelog/unreleased_20/12216.rst @@ -0,0 +1,9 @@ +.. change:: + :tags: bug, orm + :tickets: 12216 + + Fixed issue in event system which prevented an event listener from being + attached and detached from multiple class-like objects, namely the + :class:`.sessionmaker` or :class:`.scoped_session` targets that assign to + :class:`.Session` subclasses. + diff --git a/lib/sqlalchemy/event/registry.py b/lib/sqlalchemy/event/registry.py index 77fea0006f..d7e4b32155 100644 --- a/lib/sqlalchemy/event/registry.py +++ b/lib/sqlalchemy/event/registry.py @@ -154,7 +154,11 @@ def _removed_from_collection( if owner_ref in _collection_to_key: listener_to_key = _collection_to_key[owner_ref] - listener_to_key.pop(listen_ref) + # see #12216 - this guards against a removal that already occurred + # here. however, I cannot come up with a test that shows any negative + # side effects occurring from this removal happening, even though an + # event key may still be referenced from a clsleveldispatch here + listener_to_key.pop(listen_ref, None) def _stored_in_collection_multi( diff --git a/test/base/test_events.py b/test/base/test_events.py index 6f8456274f..7a387e8440 100644 --- a/test/base/test_events.py +++ b/test/base/test_events.py @@ -1271,6 +1271,107 @@ class RemovalTest(TearDownLocalEventsFixture, fixtures.TestBase): return Target + def test_two_subclasses_one_event(self): + """test #12216""" + + Target = self._fixture() + + class TargetSubclassOne(Target): + pass + + class TargetSubclassTwo(Target): + pass + + m1 = Mock() + + def my_event_one(x, y): + m1.my_event_one(x, y) + + event.listen(TargetSubclassOne, "event_one", my_event_one) + event.listen(TargetSubclassTwo, "event_one", my_event_one) + + t1 = TargetSubclassOne() + t2 = TargetSubclassTwo() + + t1.dispatch.event_one("x1a", "y1a") + t2.dispatch.event_one("x2a", "y2a") + + eq_( + m1.mock_calls, + [call.my_event_one("x1a", "y1a"), call.my_event_one("x2a", "y2a")], + ) + + event.remove(TargetSubclassOne, "event_one", my_event_one) + + t1.dispatch.event_one("x1b", "y1b") + t2.dispatch.event_one("x2b", "y2b") + + eq_( + m1.mock_calls, + [ + call.my_event_one("x1a", "y1a"), + call.my_event_one("x2a", "y2a"), + call.my_event_one("x2b", "y2b"), + ], + ) + + event.remove(TargetSubclassTwo, "event_one", my_event_one) + + t1.dispatch.event_one("x1c", "y1c") + t2.dispatch.event_one("x2c", "y2c") + + eq_( + m1.mock_calls, + [ + call.my_event_one("x1a", "y1a"), + call.my_event_one("x2a", "y2a"), + call.my_event_one("x2b", "y2b"), + ], + ) + + def test_two_subclasses_one_event_reg_cleanup(self): + """test #12216""" + + from sqlalchemy.event import registry + + Target = self._fixture() + + class TargetSubclassOne(Target): + pass + + class TargetSubclassTwo(Target): + pass + + m1 = Mock() + + def my_event_one(x, y): + m1.my_event_one(x, y) + + event.listen(TargetSubclassOne, "event_one", my_event_one) + event.listen(TargetSubclassTwo, "event_one", my_event_one) + + key1 = (id(TargetSubclassOne), "event_one", id(my_event_one)) + key2 = (id(TargetSubclassTwo), "event_one", id(my_event_one)) + + assert key1 in registry._key_to_collection + assert key2 in registry._key_to_collection + + del TargetSubclassOne + gc_collect() + + # the key remains because the gc routine would be based on deleting + # Target (I think) + assert key1 in registry._key_to_collection + assert key2 in registry._key_to_collection + + del TargetSubclassTwo + gc_collect() + + assert key1 in registry._key_to_collection + assert key2 in registry._key_to_collection + + # event.remove(TargetSubclassTwo, "event_one", my_event_one) + def test_clslevel(self): Target = self._fixture() @@ -1503,6 +1604,38 @@ class RemovalTest(TearDownLocalEventsFixture, fixtures.TestBase): assert key not in registry._key_to_collection assert collection_ref not in registry._collection_to_key + @testing.requires.predictable_gc + def test_listener_collection_removed_cleanup_clslevel(self): + """test related to #12216""" + + from sqlalchemy.event import registry + + Target = self._fixture() + + m1 = Mock() + + event.listen(Target, "event_one", m1) + + key = (id(Target), "event_one", id(m1)) + + assert key in registry._key_to_collection + collection_ref = list(registry._key_to_collection[key])[0] + assert collection_ref in registry._collection_to_key + + t1 = Target() + t1.dispatch.event_one("t1") + + del t1 + + del Target + + gc_collect() + + # gc of a target class does not currently cause these collections + # to be cleaned up + assert key in registry._key_to_collection + assert collection_ref in registry._collection_to_key + def test_remove_not_listened(self): Target = self._fixture()