From 368f5ac6668456609cf5f862c0676108cef4bec8 Mon Sep 17 00:00:00 2001 From: Mike Bayer Date: Tue, 28 Oct 2025 16:20:54 -0400 Subject: [PATCH] try not to rely on GC working at all for tests GC seems to be working in fewer cases across different interpreters, so add a new step to force an InstanceState to act as though it got dereferenced, for those cases where we have del'ed the object, done a gc collect, and we know it should not be reachable. Change-Id: Ie97af1d5739f8d5a24aabe1296f14ce99ec1b0e5 --- lib/sqlalchemy/orm/state.py | 25 ++++ test/aaa_profiling/test_memusage.py | 209 ++++++++++++++-------------- test/orm/test_hasparent.py | 38 ++--- 3 files changed, 134 insertions(+), 138 deletions(-) diff --git a/lib/sqlalchemy/orm/state.py b/lib/sqlalchemy/orm/state.py index 34575c56b8..b132f760a8 100644 --- a/lib/sqlalchemy/orm/state.py +++ b/lib/sqlalchemy/orm/state.py @@ -512,6 +512,31 @@ class InstanceState(interfaces.InspectionAttrInfo, Generic[_O]): # used by the test suite, apparently self._detach() + def _force_dereference(self) -> None: + """Force this InstanceState to act as though its weakref has + been GC'ed. + + this is used for test code that has to test reactions to objects + being GC'ed. We can't reliably force GCs to happen under all + CI circumstances. + + """ + + # if _strong_obj is set, then our object would not be getting + # GC'ed (at least within the scope of what we use this for in tests). + # so make sure this is not set + assert self._strong_obj is None + + obj = self.obj() + if obj is None: + # object was GC'ed and we're done! woop + return + + del obj + + self._cleanup(self.obj) + self.obj = lambda: None # type: ignore + def _cleanup(self, ref: weakref.ref[_O]) -> None: """Weakref callback cleanup. diff --git a/test/aaa_profiling/test_memusage.py b/test/aaa_profiling/test_memusage.py index 47aa5f94b0..e1c06ec44c 100644 --- a/test/aaa_profiling/test_memusage.py +++ b/test/aaa_profiling/test_memusage.py @@ -1778,30 +1778,9 @@ class MiscMemoryIntensiveTests(fixtures.TestBase): s.commit() -# these tests simply cannot run reliably on github actions machines. -# even trying ten times, they sometimes can't proceed. -@testing.add_to_marker.gc_intensive class WeakIdentityMapTest(_fixtures.FixtureTest): run_inserts = None - def run_up_to_n_times(self, fn, times): - error = None - for _ in range(times): - try: - fn() - except Exception as err: - error = err - with testing.db.begin() as conn: - conn.execute(self.tables.addresses.delete()) - conn.execute(self.tables.users.delete()) - continue - else: - break - else: - if error: - raise error - - @testing.requires.predictable_gc def test_weakref(self): """test the weak-referencing identity map, which strongly- references modified items.""" @@ -1809,83 +1788,86 @@ class WeakIdentityMapTest(_fixtures.FixtureTest): users, User = self.tables.users, self.classes.User self.mapper_registry.map_imperatively(User, users) - def go(): - with Session(testing.db) as s: - gc_collect() + with Session(testing.db) as s: + gc_collect() - s.add(User(name="ed")) - s.flush() - assert not s.dirty + s.add(User(name="ed")) + s.flush() + assert not s.dirty - user = s.query(User).one() + user = s.query(User).one() - # heisenberg the GC a little bit, since #7823 caused a lot more - # GC when mappings are set up, larger test suite started - # failing on this being gc'ed - user_is = user._sa_instance_state - del user - gc_collect() - gc_collect() - gc_collect() - assert user_is.obj() is None + user_is = user._sa_instance_state + del user + user_is._force_dereference() - assert len(s.identity_map) == 0 + assert len(s.identity_map) == 0 - user = s.query(User).one() - user.name = "fred" - del user - gc_collect() - assert len(s.identity_map) == 1 - assert len(s.dirty) == 1 - assert None not in s.dirty - s.flush() - gc_collect() - assert not s.dirty - assert not s.identity_map + user = s.query(User).one() + user_is = user._sa_instance_state + user.name = "fred" - user = s.query(User).one() - assert user.name == "fred" - assert s.identity_map + del user + gc_collect() - self.run_up_to_n_times(go, 10) + # object was not gc'ed + assert user_is.obj() + + assert len(s.identity_map) == 1 + assert len(s.dirty) == 1 + assert None not in s.dirty + s.flush() + gc_collect() + user_is._force_dereference() + + assert not s.dirty + assert not s.identity_map + + user = s.query(User).one() + assert user.name == "fred" + assert s.identity_map - @testing.requires.predictable_gc def test_weakref_pickled(self): users, User = self.tables.users, pickleable.User self.mapper_registry.map_imperatively(User, users) - def go(): - with Session(testing.db) as s: - gc_collect() + with Session(testing.db) as s: + gc_collect() - s.add(User(name="ed")) - s.flush() - assert not s.dirty + s.add(User(name="ed")) + s.flush() + assert not s.dirty - user = s.query(User).one() - user.name = "fred" - s.expunge(user) + user = s.query(User).one() + user.name = "fred" + s.expunge(user) - u2 = pickle.loads(pickle.dumps(user)) + u2 = pickle.loads(pickle.dumps(user)) - del user - s.add(u2) + uis = user._sa_instance_state + u2is = u2._sa_instance_state - del u2 - gc_collect() + del user + uis._force_dereference() - assert len(s.identity_map) == 1 - assert len(s.dirty) == 1 - assert None not in s.dirty - s.flush() - gc_collect() - assert not s.dirty + s.add(u2) - assert not s.identity_map + del u2 - self.run_up_to_n_times(go, 10) + # was not gced + assert u2is.obj() is not None + assert len(s.identity_map) == 1 + assert len(s.dirty) == 1 + assert None not in s.dirty + s.flush() + + gc_collect() + u2is._force_dereference() + + assert not s.dirty + + assert not s.identity_map - @testing.requires.predictable_gc def test_weakref_with_cycles_o2m(self): Address, addresses, users, User = ( self.classes.Address, @@ -1900,43 +1882,42 @@ class WeakIdentityMapTest(_fixtures.FixtureTest): properties={"addresses": relationship(Address, backref="user")}, ) self.mapper_registry.map_imperatively(Address, addresses) - gc_collect() - def go(): - with Session(testing.db) as s: - s.add( - User(name="ed", addresses=[Address(email_address="ed1")]) - ) - s.commit() + with Session(testing.db) as s: + s.add(User(name="ed", addresses=[Address(email_address="ed1")])) + s.commit() - user = s.query(User).options(joinedload(User.addresses)).one() - user.addresses[0].user # lazyload - eq_( - user, - User(name="ed", addresses=[Address(email_address="ed1")]), - ) + user = s.query(User).options(joinedload(User.addresses)).one() + user.addresses[0].user # lazyload + eq_( + user, + User(name="ed", addresses=[Address(email_address="ed1")]), + ) - del user - gc_collect() - assert len(s.identity_map) == 0 + del user + gc_collect() + assert len(s.identity_map) == 0 - user = s.query(User).options(joinedload(User.addresses)).one() - user.addresses[0].email_address = "ed2" - user.addresses[0].user # lazyload - del user - gc_collect() - assert len(s.identity_map) == 2 + user = s.query(User).options(joinedload(User.addresses)).one() + uis = user._sa_instance_state - s.commit() - user = s.query(User).options(joinedload(User.addresses)).one() - eq_( - user, - User(name="ed", addresses=[Address(email_address="ed2")]), - ) + user.addresses[0].email_address = "ed2" + user.addresses[0].user # lazyload + del user + gc_collect() - self.run_up_to_n_times(go, 10) + # was not GC'ed + assert uis.obj() is not None + + assert len(s.identity_map) == 2 + + s.commit() + user = s.query(User).options(joinedload(User.addresses)).one() + eq_( + user, + User(name="ed", addresses=[Address(email_address="ed2")]), + ) - @testing.requires.predictable_gc def test_weakref_with_cycles_o2o(self): Address, addresses, users, User = ( self.classes.Address, @@ -1954,7 +1935,6 @@ class WeakIdentityMapTest(_fixtures.FixtureTest): }, ) self.mapper_registry.map_imperatively(Address, addresses) - gc_collect() s.add(User(name="ed", address=Address(email_address="ed1"))) s.commit() @@ -1963,16 +1943,29 @@ class WeakIdentityMapTest(_fixtures.FixtureTest): user.address.user eq_(user, User(name="ed", address=Address(email_address="ed1"))) + uis = user._sa_instance_state + ais = user.address._sa_instance_state del user gc_collect() + + # in case gc collect didn't work, force a dereference + uis._force_dereference() + ais._force_dereference() + + # identity map is cleaned out assert len(s.identity_map) == 0 user = s.query(User).options(joinedload(User.address)).one() + uis = user._sa_instance_state user.address.email_address = "ed2" user.address.user # lazyload del user gc_collect() + + # was not gc'ed + assert uis.obj() is not None + assert len(s.identity_map) == 2 s.commit() diff --git a/test/orm/test_hasparent.py b/test/orm/test_hasparent.py index 6332a09f4c..98662b4004 100644 --- a/test/orm/test_hasparent.py +++ b/test/orm/test_hasparent.py @@ -107,24 +107,22 @@ class ParentRemovalTest(fixtures.MappedTest): self._assert_not_hasparent(a1) - @testing.add_to_marker.gc_intensive - @testing.requires.predictable_gc def test_stale_state_positive_gc(self): User = self.classes.User s, u1, a1 = self._fixture() s.expunge(u1) - del u1 - gc_collect() + + u1_is = u1._sa_instance_state + # act as though GC happened + u1_is._force_dereference() u1 = s.query(User).first() u1.addresses.remove(a1) self._assert_not_hasparent(a1) - @testing.add_to_marker.gc_intensive @testing.requires.updateable_autoincrement_pks - @testing.requires.predictable_gc def test_stale_state_positive_pk_change(self): """Illustrate that we can't easily link a stale state to a fresh one if the fresh one has @@ -139,9 +137,6 @@ class ParentRemovalTest(fixtures.MappedTest): s._expunge_states([attributes.instance_state(u1)]) - del u1 - gc_collect() - u1 = s.query(User).first() # primary key change. now we @@ -169,8 +164,6 @@ class ParentRemovalTest(fixtures.MappedTest): self._assert_not_hasparent(a1) - @testing.add_to_marker.gc_intensive - @testing.requires.predictable_gc def test_stale_state_negative_child_expired(self): """illustrate the current behavior of expiration on the child. @@ -189,15 +182,9 @@ class ParentRemovalTest(fixtures.MappedTest): u1.addresses.remove(a1) u2_is = u2._sa_instance_state - del u2 - for i in range(5): - gc_collect() - # heisenberg the GC a little bit, since #7823 caused a lot more - # GC when mappings are set up, larger test suite started failing - # on this being gc'ed - o = u2_is.obj() - assert o is None + # act as though GC happened + u2_is._force_dereference() # controversy here. The action is # to expire one object, not the other, and remove; @@ -208,8 +195,6 @@ class ParentRemovalTest(fixtures.MappedTest): self._assert_not_hasparent(a1) # self._assert_hasparent(a1) - @testing.add_to_marker.gc_intensive - @testing.requires.predictable_gc def test_stale_state_negative(self): User = self.classes.User s, u1, a1 = self._fixture() @@ -221,15 +206,8 @@ class ParentRemovalTest(fixtures.MappedTest): s._expunge_states([attributes.instance_state(u2)]) u2_is = u2._sa_instance_state - del u2 - - for i in range(5): - gc_collect() - # heisenberg the GC a little bit, since #7823 caused a lot more - # GC when mappings are set up, larger test suite started failing - # on this being gc'ed - o = u2_is.obj() - assert o is None + + u2_is._force_dereference() assert_raises_message( orm_exc.StaleDataError, -- 2.47.3