scoped_session’s scopefunc uses LocalStack() instead of AppContext()

[torotil]

Whenever multiple app contexts are pushed to the context stack the session handling fails. Although this is not a likely use-case outside of tests it is super-hard to debug when it happens.

The code for handling the session life-time seems to assume that it should remove a session whenever an app context is teared down. With connection_stack.__ident_func__ as scopefunc for the scoped_session we don’t get one session per app context but one session per thread. That is because the function references the stack itself and not the app context on the stack.

This becomes relevant when more than one app-context is pushed to the stack. In such cases the session is removed when the innermost app context is destroyed. This then can lead to various different errors in the code relying on the outer app context’s session.

This is especially hard to debug, because everything in Flask-SQLAlchemy looks like the session is handled per app-context while this is actually not the case.

---

For context here are the two relevant comments of #379 . Since the issue there is closed I can’t post there so I’m opening this new issue.

also I dont get the design behind the choice of scopefunc, by default scopefunc is connection_stack.__ident_func__ which is effectively implemented with thread.get_ident, means that app would get same session within same thread, no matter how many app context on the stack, I think scopefunc = lambda: id(flask_sqlalchemy.connection_stack.top) is more desirable. — @georgexsh

and the answer to this was was

I'm going to need an example of how to run Flask in such a way that the same app context is reused for multiple requests (not for multiple manual request contexts within a single app context or test, but for actual requests). I can't reproduce this error and using the app context should be correct. The app context is for global app state during an interaction (such as a request or cli command). — @davidism

[davidism]

As you quoted, please add a minimal reproducible example.

[torotil]

Here is an example code.

import flask
import flask_sqlalchemy
import pytest

app = flask.Flask(__name__)
app.config["SQLALCHEMY_DATABASE_URI"] = "postgresql:///test"
db = flask_sqlalchemy.SQLAlchemy(app)


class Event(db.Model):
    id = db.Column(db.Integer(), primary_key=True)


@app.route("/event", methods=("POST", ))
def create_event():
    # Force using a new DB session (eg. running a celery task in the same thread for testing).
    with app.app_context():
        event = Event()
        db.session.add(event)
        db.session.commit()
        return flask.jsonify({"id": event.id})


@pytest.fixture(name="app_context")
def fixture_app_context():
    with app.app_context():
        yield


@pytest.fixture(name="db")
def fixture_db():
    db.create_all()
    yield db
    db.session.remove()
    db.drop_all()


def test_create(app_context, db):
    assert Event.query.all() == []
    event = Event()
    db.session.add(event)
    db.session.commit()
    assert [e.id for e in Event.query.all()] == [1]
    with app.test_client() as client:
        response = client.post("/event")
    assert response.status_code == 200
    assert response.json == {"id": 2}
    assert event.id == 1

which fails with

sqlalchemy.orm.exc.DetachedInstanceError: Instance <Event at 0x7fa273eac7f0> is not bound to a Session; attribute refresh operation cannot proceed (Background on this error at: http://sqlalche.me/e/13/bhk3)

[davidism]

Duplicate of #508. https://github.com/pallets-eco/flask-sqlalchemy/pull/1087 changes the session scope to use the current app context instead of the thread. If there are different contexts pushed, there is a different session for each context.