Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AssociationProxy becomes useless when parent goes out of scope #4268

Closed
sqlalchemy-bot opened this issue Jun 1, 2018 · 7 comments
Closed
Labels
bug Something isn't working low priority sqlalchemy.ext extension modules, most of which are ORM related
Milestone

Comments

@sqlalchemy-bot
Copy link
Collaborator

Migrated issue, originally created by Chris Wilson (@qris1)

This fails:

from sqlalchemy import Column, ForeignKey, Integer, Table, Text, create_engine
from sqlalchemy.ext.associationproxy import association_proxy
from sqlalchemy.ext.declarative import declarative_base
from sqlalchemy.orm import relationship, sessionmaker

Base = declarative_base()

userkeywords_table = Table('userkeywords', Base.metadata,
    Column('user_id', Integer, ForeignKey("user.id"),
           primary_key=True),
    Column('keyword_id', Integer, ForeignKey("keyword.id"),
           primary_key=True)
)

class User(Base):
    __tablename__ = 'user'
    id = Column(Integer, primary_key=True)
    name = Column(Text)
    kw = relationship("Keyword", secondary=lambda: userkeywords_table)

    def __init__(self, name):
        self.name = name

    # proxy the 'keyword' attribute from the 'kw' relationship
    keywords = association_proxy('kw', 'keyword')
    
class Keyword(Base):
    __tablename__ = 'keyword'
    id = Column(Integer, primary_key=True)
    keyword = Column('keyword', Text)

    def __init__(self, keyword):
        self.keyword = keyword

engine = create_engine('sqlite://')
engine.echo = True
Base.metadata.create_all(engine)

DBSession = sessionmaker(bind=engine)
session = DBSession(autocommit=True)

with session.begin():
    user = User('jek')
    user.keywords.append('cheese inspector')
    session.add(user)
    user = None
    
with session.begin():
    print(session.query(User).one().keywords)

Because User is garbage-collected before the association can be read:

File untitled0.py, line 60, in : print(session.query(User).one().keywords) 
File ...\sqlalchemy-1.2.7-py3.6-win-amd64.egg\sqlalchemy\ext\associationproxy.py, line 780, in __repr__ : return repr(list(self)) 
File ...\sqlalchemy-1.2.7-py3.6-win-amd64.egg\sqlalchemy\ext\associationproxy.py, line 583, in __len__ : return len(self.col) 
File ...\sqlalchemy-1.2.7-py3.6-win-amd64.egg\sqlalchemy\ext\associationproxy.py, line 580, in : col = property(lambda self: self.lazy_collection()) 
File ...\sqlalchemy-1.2.7-py3.6-win-amd64.egg\sqlalchemy\ext\associationproxy.py, line 536, in __call__ : "stale association proxy, parent object has gone out of " 
sqlalchemy.exc.InvalidRequestError: stale association proxy, parent object has gone out of scope

Whereas if you forcibly keep a reference in a variable, then it works:

user = session.query(User).one()
print(user.keywords)

Other people have run into this issue as well:

The latter has some explanation as to why we only keep a weakref:

As a proxy, it doesn't actually store the collection that
it proxies, because that collection itself might not have been loaded
yet, or can be expired, replaced, etc. and it's just less complicated
for it to look at whatever collection is currently on the parent.

If the parent object is lost, and you still are pointing to the proxied
collection, it can't give you the collection anymore. The association
proxy stores only a weak reference to the parent... I would guess that's
just to prevent reference cycles; I tried adding a strong ref and of
course all the tests still pass, so I'm not totally sure why that was so
important here, but there you go.

Preventing reference cycles doesn't seem like a good reason to have such counter-intuitive behaviour. Also we may not need to hold a strong reference to the underlying association. A strong reference to the parent of that association should be enough to prevent it from going out of scope, keeping the association object accessible/constructable.

@sqlalchemy-bot
Copy link
Collaborator Author

Michael Bayer (@zzzeek) wrote:

As I'm sure you can see, the design here is not by accident. However, it is an extremely old use case which I've not tested recently. After extensive searching I've located the origin of the weakref is in 84420a1 and it does seem to include modifications to tests, however I can confirm adding a strong ref does not fail current tests. The original issue is #597. The issue described in #597 is not reproducible if I include a simple strong ref inside of _lazy_collection.

A suite of tests that is guaranteed to fail differently whether or not there is a strong ref can be added as follows, illustrating where the inconsistency can go. Since the "dynamic" collection is also holding onto the parent object that sets precedent to revert the weak reference. if it fails in the wild then we'll get a nice new test case however it may mean we'd have to revert the change again:

from sqlalchemy import Integer, String, ForeignKey, Column, create_engine
from sqlalchemy.orm import Session, relationship
from sqlalchemy.ext.declarative import declarative_base
from sqlalchemy.ext.associationproxy import association_proxy

import unittest

Base = declarative_base()


class A(Base):
    __tablename__ = 'a'

    id = Column(Integer, primary_key=True)
    data = Column(String)
    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)


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

s = Session(e)
s.add_all([
    A(bs=[B(data='b1'), B(data='b2')]),
    A(bs=[B(data='b3'), B(data='b4')])])
s.commit()
s.close()


class AssocProxyGCTests(unittest.TestCase):
    def test_plain_collection_gc(self):
        s = Session(e)
        a1 = s.query(A).filter_by(id=1).one()

        a1bs = a1.bs  # noqa

        del a1

        import gc
        gc.collect()

        assert (A, (1, ), None) not in s.identity_map

    def test_dynamic_collection_gc(self):
        s = Session(e)
        a1 = s.query(A).filter_by(id=1).one()

        a1bs = a1.b_dyn  # noqa

        del a1

        import gc
        gc.collect()

        # also fails, AppenderQuery holds onto parent
        assert (A, (1, ), None) not in s.identity_map

    def test_associated_collection_gc(self):
        s = Session(e)
        a1 = s.query(A).filter_by(id=1).one()

        a1bs = a1.b_data # noqa

        del a1

        import gc
        gc.collect()

        assert (A, (1, ), None) not in s.identity_map

    def test_associated_dynamic_gc(self):
        s = Session(e)
        a1 = s.query(A).filter_by(id=1).one()

        a1bs = a1.b_dynamic_data # noqa

        del a1

        import gc
        gc.collect()

        assert (A, (1, ), None) not in s.identity_map

    def test_plain_collection_iterate(self):
        s = Session(e)
        a1 = s.query(A).filter_by(id=1).one()

        a1bs = a1.bs

        del a1

        import gc
        gc.collect()

        assert len(a1bs) == 2

    def test_dynamic_collection_iterate(self):
        s = Session(e)
        a1 = s.query(A).filter_by(id=1).one()

        a1bs = a1.b_dyn  # noqa

        del a1

        import gc
        gc.collect()

        assert len(list(a1bs)) == 2

    def test_associated_collection_iterate(self):
        s = Session(e)
        a1 = s.query(A).filter_by(id=1).one()

        a1bs = a1.b_data

        del a1

        import gc
        gc.collect()

        assert len(a1bs) == 2

    def test_associated_dynamic_iterate(self):
        s = Session(e)
        a1 = s.query(A).filter_by(id=1).one()

        a1bs = a1.b_dynamic_data

        del a1

        import gc
        gc.collect()

        assert len(a1bs) == 2


if __name__ == '__main__':
    unittest.main()

@sqlalchemy-bot
Copy link
Collaborator Author

Michael Bayer (@zzzeek) wrote:

Preventing reference cycles doesn't seem like a good reason to have such counter-intuitive behaviour.

you might be surprised how upset people get when we add reference cycles to their objects. although in modern uses I'm not sure we are fully preventing against cycles in all cases now.

@sqlalchemy-bot
Copy link
Collaborator Author

@sqlalchemy-bot
Copy link
Collaborator Author

Changes by Michael Bayer (@zzzeek):

  • set milestone to "1.3"

@sqlalchemy-bot
Copy link
Collaborator Author

Michael Bayer (@zzzeek) wrote:

Strong reference parent object in association proxy

Considering the reversal of #597 as well as
84420a1 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

11947c3

@sqlalchemy-bot
Copy link
Collaborator Author

Changes by Michael Bayer (@zzzeek):

  • changed status to closed

@sqlalchemy-bot
Copy link
Collaborator Author

Michael Bayer (@zzzeek) wrote:

so keep in mind, I'm expecting people might complain about this. But since I have no recent data on that, and the original issue doesn't reproduce, I'm actually merging it into 1.3 just so we can collect more data on this, it might have to be something that is configurable. Hopefully if people have a problem with it, it can be worked out within the 1.3 betas.

@sqlalchemy-bot sqlalchemy-bot added bug Something isn't working sqlalchemy.ext extension modules, most of which are ORM related low priority labels Nov 27, 2018
@sqlalchemy-bot sqlalchemy-bot added this to the 1.3 milestone Nov 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working low priority sqlalchemy.ext extension modules, most of which are ORM related
Projects
None yet
Development

No branches or pull requests

1 participant