persistent_to_deleted does not accommodate for garbage collected object

[Mike Bayer]

diff --git a/test/orm/test_events.py b/test/orm/test_events.py
index ab61077..0a11c12 100644
--- a/test/orm/test_events.py
+++ b/test/orm/test_events.py
@@ -1872,6 +1872,31 @@ class SessionLifecycleEventsTest(_RemoveListeners, _fixtures.FixtureTest):
             ]
         )

+    def test_persistent_to_deleted_del(self):
+        sess, User, start_events = self._fixture()
+
+        u1 = User(name='u1')
+        sess.add(u1)
+        sess.flush()
+
+        listener = start_events()
+
+        @event.listens_for(sess, "persistent_to_deleted")
+        def persistent_to_deleted(session, instance):
+            assert instance is not None
+
+        sess.delete(u1)
+        del u1
+
+        sess.flush()
+
+        eq_(
+            listener.mock_calls,
+            [
+                call.persistent_to_deleted(sess, None),  # should we send state here?
+            ]
+        )
+
     def test_detached_to_persistent_via_cascaded_delete(self):
         sess, User, Address, start_events = self._fixture(include_address=True)

this is critical for 1.1 because the API is broken. either we don't call the event or we send the state (And should we change the signature so that we know if state is there or not? propose send both object and state).

fails because the object is gone. the event signature here is broken, we can't always send the object along. there might be some other events that have this also, the object would have to be persistent first and not be subject to strong_ref. persistent_to_transient for example seems to have a strong ref.

[Mike Bayer]

https://gerrit.sqlalchemy.org/200

[Mike Bayer]

OK apparently I tested downstream reporter's fix incorrectly, we do have a strong ref here inside of self._deleted. Going to keep the signature change anyway.

[Mike Bayer]

There's some internal debate here over if the object can really be None in other cases. I'm not sure it's something we'd be able to unit test because it requires getting gc.collect() to run inside of loops at specific times. But here is a way to show a None happening for persistent_to_detached, and I wonder if the approach should be, just skip the event if the object was garbage collected. This would assume that all the cases where obj is None are where we are basically ready to call the event and there's a late collection.

script:

diff --git a/lib/sqlalchemy/orm/state.py b/lib/sqlalchemy/orm/state.py
index 2704367..94415f4 100644
--- a/lib/sqlalchemy/orm/state.py
+++ b/lib/sqlalchemy/orm/state.py
@@ -316,6 +316,7 @@ class InstanceState(interfaces.InspectionAttr):
         persistent_to_transient = \
             session.dispatch.persistent_to_transient or None

+        import gc
         for state in states:
             deleted = state._deleted
             pending = state.key is None
@@ -323,6 +324,8 @@ class InstanceState(interfaces.InspectionAttr):

             state.session_id = None

+            gc.collect()
+
             if to_transient and state.key:
                 del state.key
             if persistent:

import gc
gc.disable()

from sqlalchemy import event
from sqlalchemy import *
from sqlalchemy.orm import *
from sqlalchemy.ext.declarative import declarative_base

Base = declarative_base()


class A(Base):
    __tablename__ = 'a'
    id = Column(Integer, primary_key=True)

e = create_engine("sqlite://", echo=True)
Base.metadata.create_all(e)

s = Session(e)
s.add(A(id=1))
s.commit()

a1 = s.query(A).first()
a1.foobar = a1


@event.listens_for(s, "persistent_to_detached")
def inst(session, obj):
    print "OBJ!", obj
    assert obj is not None

del a1


s.close()

[Mike Bayer]

also. if the user expects every object to go through every lifecycle, and they aren't keeping strong refs to the objects they care about, then they can't be guaranteed every event is called in any case. that is, does it really matter if the object were gc'ed before we start calling events, or in the middle of it.

[Mike Bayer]

ok try again https://gerrit.sqlalchemy.org/#/c/202/

[Mike Bayer]

Ensure strong ref to obj before calling persistent_to_deleted, others

Add checks in spots where state.obj() might be late-GC'ed before we get a chance to call the event. There may be more cases of these which we should address as they come up. The Session should always be maintaining strong refs to objects that have pending operations left on them, so for these cases we need to ensure that ref remains long enough for the event to be called.

Change-Id: I1a7c7bc57130acc11f54ad55924af2e36ac75101 Fixes: #3808

→ <<cset 728ce8cc480d>>