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

diff --git a/doc/build/changelog/unreleased_20/12216.rst b/doc/build/changelog/unreleased_20/12216.rst
new file mode 100644 (file)
index 0000000..a412673
--- /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 77fea0006f4f7e470a0f8c9c83105f0cb1d81b58..d7e4b321553752f702c4309c9e558b7a0a919d7f 100644 (file)
--- 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 6f8456274f3bc7a76b486ad52945dae0a7241212..7a387e8440dd9783748e53343c39cda03be826c9 100644 (file)
--- 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()