From: Mike Bayer Date: Fri, 1 Jun 2018 19:50:25 +0000 (-0500) Subject: Strong reference parent object in association proxy X-Git-Tag: rel_1_3_0b1~58^2 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=11947c3f1fce71a8b383eb1fc9af28a0ee0fdb80;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git Strong reference parent object in association proxy Considering the reversal of #597 as well as 84420a1d0fe09d7a45e878e853aa9f5258561f8b as I am unable to reproduce the original issues from that release. The long-standing behavior of the association proxy collection maintaining only a weak reference to the parent object is reverted; the proxy will now maintain a strong reference to the parent for as long as the proxy collection itself is also in memory, eliminating the "stale association proxy" error. This change is being made on an experimental basis to see if any use cases arise where it causes side effects. Change-Id: I051334be90a343dd0e8a1f35e072075eb14b14a7 Fixes: #4268 --- diff --git a/doc/build/changelog/migration_13.rst b/doc/build/changelog/migration_13.rst index 5000626869..74ff549156 100644 --- a/doc/build/changelog/migration_13.rst +++ b/doc/build/changelog/migration_13.rst @@ -243,6 +243,72 @@ The fix now includes that ``address.user_id`` is left unchanged as per :ticket:`3844` +.. _change_4268: + +Association Proxy now Strong References the Parent Object +========================================================= + +The long-standing behavior of the association proxy collection maintaining +only a weak reference to the parent object is reverted; the proxy will now +maintain a strong reference to the parent for as long as the proxy +collection itself is also in memory, eliminating the "stale association +proxy" error. This change is being made on an experimental basis to see if +any use cases arise where it causes side effects. + +As an example, given a mapping with association proxy:: + + class A(Base): + __tablename__ = 'a' + + id = Column(Integer, primary_key=True) + bs = relationship("B") + b_data = association_proxy('bs', 'data') + + + class B(Base): + __tablename__ = 'b' + id = Column(Integer, primary_key=True) + a_id = Column(ForeignKey("a.id")) + data = Column(String) + + + a1 = A(bs=[B(data='b1'), B(data='b2')]) + + b_data = a1.b_data + +Previously, if ``a1`` were deleted out of scope:: + + del a1 + +Trying to iterate the ``b_data`` collection after ``a1`` is deleted from scope +would raise the error ``"stale association proxy, parent object has gone out of +scope"``. This is because the association proxy needs to access the actual +``a1.bs`` collection in order to produce a view, and prior to this change it +maintained only a weak reference to ``a1``. In particular, users would +frequently encounter this error when performing an inline operation +such as:: + + collection = session.query(A).filter_by(id=1).first().b_data + +Above, because the ``A`` object would be garbage collected before the +``b_data`` collection were actually used. + +The change is that the ``b_data`` collection is now maintaining a strong +reference to the ``a1`` object, so that it remains present:: + + assert b_data == ['b1', 'b2'] + +This change introduces the side effect that if an application is passing around +the collection as above, **the parent object won't be garbage collected** until +the collection is also discarded. As always, if ``a1`` is persistent inside a +particular :class:`.Session`, it will remain part of that session's state +until it is garbage collected. + +Note that this change may be revised if it leads to problems. + + +:ticket:`4268` + New Features and Improvements - Core ==================================== diff --git a/doc/build/changelog/unreleased_13/4268.rst b/doc/build/changelog/unreleased_13/4268.rst new file mode 100644 index 0000000000..3857313575 --- /dev/null +++ b/doc/build/changelog/unreleased_13/4268.rst @@ -0,0 +1,15 @@ +.. change:: + :tags: bug, ext + :tickets: 4268 + + The long-standing behavior of the association proxy collection maintaining + only a weak reference to the parent object is reverted; the proxy will now + maintain a strong reference to the parent for as long as the proxy + collection itself is also in memory, eliminating the "stale association + proxy" error. This change is being made on an experimental basis to see if + any use cases arise where it causes side effects. + + .. seealso:: + + :ref:`change_4268` + diff --git a/lib/sqlalchemy/ext/associationproxy.py b/lib/sqlalchemy/ext/associationproxy.py index acf13df46a..1c28b10a17 100644 --- a/lib/sqlalchemy/ext/associationproxy.py +++ b/lib/sqlalchemy/ext/associationproxy.py @@ -14,7 +14,6 @@ See the example ``examples/association/proxied_association.py``. """ import operator -import weakref from .. import exc, orm, util from ..orm import collections, interfaces from ..sql import or_ @@ -637,22 +636,17 @@ class AssociationProxyInstance(object): class _lazy_collection(object): def __init__(self, obj, target): - self.ref = weakref.ref(obj) + self.parent = obj self.target = target def __call__(self): - obj = self.ref() - if obj is None: - raise exc.InvalidRequestError( - "stale association proxy, parent object has gone out of " - "scope") - return getattr(obj, self.target) + return getattr(self.parent, self.target) def __getstate__(self): - return {'obj': self.ref(), 'target': self.target} + return {'obj': self.parent, 'target': self.target} def __setstate__(self, state): - self.ref = weakref.ref(state['obj']) + self.parent = state['obj'] self.target = state['target'] diff --git a/test/ext/test_associationproxy.py b/test/ext/test_associationproxy.py index 212599d7ab..2204f13c17 100644 --- a/test/ext/test_associationproxy.py +++ b/test/ext/test_associationproxy.py @@ -2362,6 +2362,7 @@ class InfoTest(fixtures.TestBase): eq_(Foob.assoc.info, {'foo': 'bar'}) + class MultiOwnerTest(fixtures.DeclarativeMappedTest, testing.AssertsCompiledSQL): __dialect__ = 'default' @@ -2431,3 +2432,165 @@ class MultiOwnerTest(fixtures.DeclarativeMappedTest, "AND d.value = :value_1)" ) +class ScopeBehaviorTest(fixtures.DeclarativeMappedTest): + # test some GC scenarios, including issue #4268 + + @classmethod + def setup_classes(cls): + Base = cls.DeclarativeBasic + + class A(Base): + __tablename__ = 'a' + + id = Column(Integer, primary_key=True) + data = Column(String(50)) + bs = relationship("B") + + b_dyn = relationship("B", lazy="dynamic") + + b_data = association_proxy("bs", "data") + + b_dynamic_data = association_proxy("bs", "data") + + class B(Base): + __tablename__ = 'b' + + id = Column(Integer, primary_key=True) + aid = Column(ForeignKey('a.id')) + data = Column(String(50)) + + @classmethod + def insert_data(cls): + A, B = cls.classes("A", "B") + + s = Session(testing.db) + s.add_all([ + A(id=1, bs=[B(data='b1'), B(data='b2')]), + A(id=2, bs=[B(data='b3'), B(data='b4')])]) + s.commit() + s.close() + + def test_plain_collection_gc(self): + A, B = self.classes("A", "B") + + s = Session(testing.db) + a1 = s.query(A).filter_by(id=1).one() + + a1bs = a1.bs # noqa + + del a1 + + gc_collect() + + assert (A, (1, ), None) not in s.identity_map + + @testing.fails("dynamic relationship strong references parent") + def test_dynamic_collection_gc(self): + A, B = self.classes("A", "B") + + s = Session(testing.db) + + a1 = s.query(A).filter_by(id=1).one() + + a1bs = a1.b_dyn # noqa + + del a1 + + gc_collect() + + # also fails, AppenderQuery holds onto parent + assert (A, (1, ), None) not in s.identity_map + + @testing.fails("association proxy strong references parent") + def test_associated_collection_gc(self): + A, B = self.classes("A", "B") + + s = Session(testing.db) + + a1 = s.query(A).filter_by(id=1).one() + + a1bs = a1.b_data # noqa + + del a1 + + gc_collect() + + assert (A, (1, ), None) not in s.identity_map + + @testing.fails("association proxy strong references parent") + def test_associated_dynamic_gc(self): + A, B = self.classes("A", "B") + + s = Session(testing.db) + + a1 = s.query(A).filter_by(id=1).one() + + a1bs = a1.b_dynamic_data # noqa + + del a1 + + gc_collect() + + assert (A, (1, ), None) not in s.identity_map + + def test_plain_collection_iterate(self): + A, B = self.classes("A", "B") + + s = Session(testing.db) + + a1 = s.query(A).filter_by(id=1).one() + + a1bs = a1.bs + + del a1 + + gc_collect() + + assert len(a1bs) == 2 + + def test_dynamic_collection_iterate(self): + A, B = self.classes("A", "B") + + s = Session(testing.db) + + a1 = s.query(A).filter_by(id=1).one() + + a1bs = a1.b_dyn # noqa + + del a1 + + gc_collect() + + assert len(list(a1bs)) == 2 + + def test_associated_collection_iterate(self): + A, B = self.classes("A", "B") + + s = Session(testing.db) + + a1 = s.query(A).filter_by(id=1).one() + + a1bs = a1.b_data + + del a1 + + gc_collect() + + assert len(a1bs) == 2 + + def test_associated_dynamic_iterate(self): + A, B = self.classes("A", "B") + + s = Session(testing.db) + + a1 = s.query(A).filter_by(id=1).one() + + a1bs = a1.b_dynamic_data + + del a1 + + gc_collect() + + assert len(a1bs) == 2 + +