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

session.remove called too early when there are stacked application contexts #508

Closed
arikfr opened this issue Jun 22, 2017 · 3 comments · Fixed by #1087
Closed

session.remove called too early when there are stacked application contexts #508

arikfr opened this issue Jun 22, 2017 · 3 comments · Fixed by #1087
Milestone

Comments

@arikfr
Copy link

arikfr commented Jun 22, 2017

Currently flask-sqlalchemy will call session.remove() when app.teardown_appcontext is called. Because application contexts can be stacked, it means that we will remove the session when the inner most application context is popped, while there still might be other active contexts.

Aren't sessions supposed to be scoped per application context?

Here's an example to show the issue:

from flask import Flask
from flask_sqlalchemy import SQLAlchemy

app = Flask(__name__)
app.config['SQLALCHEMY_DATABASE_URI'] = 'sqlite://'
db = SQLAlchemy()
db.init_app(app)


class User(db.Model):
    id = db.Column(db.Integer, primary_key=True)
    name = db.Column(db.String())


def test_session_remove_example():
    with app.app_context():
        db.create_all()

        user = User(name='a')
        db.session.add(user)
        assert user in db.session

        with app.app_context():
            assert user in db.session

        # This will fail, because the session was removed.
        assert user in db.session
@ThiefMaster
Copy link
Contributor

out of curiosity, why do you have multiple instances of the same app's context on the stack?

@arikfr
Copy link
Author

arikfr commented Jun 22, 2017

Good question :) In my background jobs, before using Flask-SQLAlchemy, I didn't need application context for all tasks, so I was explicitly pushing it for a few of the tasks that did need it (to use Flask-Mail).

After switching to Flask-SQLAlchemy, I changed the background jobs base class to push application context before running the tasks as it was always required now. This resulted in the application context being pushed twice and exposing this issue.

I can't imagine a real world scenario when this might happen, but because it does expose some unexpected behavior, I thought it's worth reporting.

@davidism
Copy link
Member

#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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging a pull request may close this issue.

4 participants