Expire session state as at the top level

[charness]

Fix "sqlalchemy.exc.InvalidRequestError: Instance XXX has been deleted." (#5)

Apply the "optional step" from the after_transaction_end listener in the "Supporting Tests with Rollbacks" sidebar of SQLAlchemy's Joining a Session into an External Transaction (such as for test suites) documentation:

    # ensure that state is expired the way
    # session.commit() at the top level normally does
    # (optional step)
    session.expire_all()

I was able to replicate the error with a tiny subset of my test case in which I originally ran into the problem, and the test completes successfully with my addition to restart_savepoint.

However, without the fix, the test hangs in the last .commit() on an apparent deadlock between a RELEASE SAVEPOINT and DROP TABLE address. If I forcibly kill the connection (SELECT pg_terminate_backend(pid) on either of the two pids), the test completes, and I see the error as reported in the issue.

While that demonstrates reproducibility and the effectiveness of the fix, the hung connection is disappointing.

If I remove the ORM attribute access or change it to id, the test neither generates the reported error nor hangs, even without the fix.

If I remove the reference from account_inst to address_inst (account_inst.addresses = []) – which would be good hygiene at the Python level – the error also does not appear.

[jeancochrane]

Thanks for this @charness! Apologies for the late reply, I somehow didn't get a notification for this. Will take a look today.

[jeancochrane]

Looks good to me! Thanks for your work on this @charness and I apologize for my delayed review.

Do you have a sense of why this works? To be honest I'm not sure what's going wrong or why it fixes the problem. I'm willing to bring it in because the official SQLAlchemy docs suggest it, so it seems safe enough, but it'd be nice to have an idea of what's going on.

[charness]

Do you have a sense of why this works?

Sorry, I've pretty well lost the context in my head.

I faintly recall when _transaction.rehydrate_object adds the object back into the session, it sets a flag in the deleted object's state such that it misses a branch somewhere that would have otherwise avoided _update_impl and its error, but that's really not much of a lead.

Looking through sqlalchemy.orm.session and sqlalchemy.orm.state just now didn't jog my memory as to which flag or branch it might have been. The state-management code in sqlalchemy is pretty dense stuff.

I share both your hesitance about a seems-to-work fix and your being reassured by the fix's coming straight from the documentation. If I get some time in the morning, I'll try harder to refresh my memory.

[jeancochrane]

Don’t worry too much about it! I only asked in case you had a firm grasp on it. Unfortunately my experience has been that working with SQLAlchemy necessarily involves a certain amount of superstitioua code, since the library internals are so complicated. If you figure it out at some point I’d love to know, but otherwise I’ll be fine staying a bit in the dark.

…

On Thu, Nov 8, 2018 at 3:09 AM Dave Charness ***@***.***> wrote: Do you have a sense of why this works? Sorry, I've pretty well lost the context in my head. I faintly recall when _transaction.rehydrate_object adds the object back into the session, it sets a flag in the deleted object's state such that it misses a branch somewhere that would have otherwise avoided _update_impl and its error, but that's really not much of a lead. Looking through sqlalchemy.orm.session and sqlalchemy.orm.state just now didn't jog my memory as to which flag or branch it might have been. The state-management code in sqlalchemy is pretty dense stuff. I share both your hesitance about a seems-to-work fix and your being reassured by the fix's coming straight from the documentation. If I get some time in the morning, I'll try harder to refresh my memory. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#6 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ANg6GqjrpnczZTmYI3_ZJ4_NBjorDLmaks5us-afgaJpZM4Wvkhp> .

[krispharper]

Was this ever released? The latest versions on GitHub and PyPI are 1.0.0.

[jeancochrane]

I don’t think so, it’s queued up for the next release. I should be able to get a new version published early next week.

…

On Thu, Feb 28, 2019 at 5:15 PM Kris Harper ***@***.***> wrote: Was this ever released? The latest versions on GitHub and PyPI are 1.0.0. — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#6 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ANg6GvjzvyZyG0vSG5whjpe0eGbSg0z1ks5vSFT9gaJpZM4Wvkhp> .

[krispharper]

That'd be awesome. I just found this project and we're looking to use in our CI pipeline. Thanks for putting it out there.

[jeancochrane]

This bugfix should be released in https://github.com/jeancochrane/pytest-flask-sqlalchemy/releases/tag/v1.0.1, which is available on PyPi now.

Thanks for your patience @krispharper, and I'm glad to hear this project might help you with CI! Feel free to open an issue if you run into any trouble with the integration.

[krispharper]

Awesome, thanks a bunch.

[killthekitten]

@charness thank you for the fix, I couldn't trace this down for a long time!

However, it introduced some flakiness in our test suite for several tests that do a commit and then check if the attribute changes. Have you had anything like it?

The code is similar to this:

def test_should_confirm_account(db_session, app_client):
    user = UserFactory()
    token = user.generate_token_and_save_timestamp()

    db_session.add(user)
    # First commit or flush in some cases
    db_session.commit()

    # One more commit here:
    response = app_client.post("/account/confirm", json={"token": token})
    assert response.status_code == 200

    # Fails with:
    # Instance <User at 0x108bf1d30> is not bound to a Session; attribute refresh operation cannot proceed (Background on this error at: http://sqlalche.me/e/bhk3 
    assert user.is_confirmed()

I could try isolate it further.

[jeancochrane]

Thanks for reporting that @killthekitten! I opened up #14 to track the bug. A fully isolated case would be awesome, but if you don't have time for that, your pip freeze output and more context on the test setup (_db and app_client fixture definitions) would be helpful too.

[killthekitten]

@jeancochrane the original issue reproduces after pinning SQLAlchemy to 1.2.18, so it's not relevant to #14 anymore. Bumping SQLAlchemy to the current version brings many more failed cases (2-3 vs 50+).

I've tried to, but couldn't reproduce it in a sandbox. Interestingly, local venv pytest run behaves differently compared to CI.

I guess the bug is somehow related to the way we stub users (authenticated_as context manager below). Anyway, here are the pieces you requested (some parts of venv and code are omitted):

factory_boy==2.11.1
Flask==1.0.2
Flask-SQLAlchemy==2.3.2
psycopg2==2.7.7
SQLAlchemy==1.2.18
werkzeug==0.14.1
pip==19.0.3
pytest==4.3.1
pytest-flask-sqlalchemy==1.0.1

# app/configuration/__init__.py

from app.configuration.db import db

__all__ = ["db"]

# tests/__init__.py

from app import create_app
from app.configuration.db import db
from app.environment import TestEnvironment

app = create_app(TestEnvironment)
app.app_context().push()


__all__ = ["app", "db"]

# conftest.py
from tests import db, app
from tests.factories import UserFactory

import pytest

# This fixture is used in part of the tests,
# however not too often
@pytest.fixture(scope="function")
def user(db_session):
    user = UserFactory()
    db_session.add(user)
    db_session.commit()

    return user

@pytest.fixture(scope="function")
def app_client():
    app_client = app.test_client()
    return app_client


@pytest.fixture(scope="session")
def _db(request):
    return db

from app.models import User
from contextlib import contextmanager
from flask import g


@contextmanager
def authenticated_as(user: User):
    g.current_user = user
    try:
        yield
    finally:
        g.current_user = None

[tool:pytest]
mocked-sessions = app.configuration.db.session