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

A single database session is used for the lifetime of the application #19

Open
sersorrel opened this issue Aug 30, 2019 · 0 comments
Open

Comments

@sersorrel
Copy link
Contributor

Each Database uses exactly one SQLAlchemy session:

Session = sessionmaker(bind=self._engine)
self._session = Session()

and the Database is shared across the whole application:

app["db"] = db = Database(c["database"])

which implies that the entire application – all async contexts – shares a session.

This is terrible!

The Session is very much intended to be used in a non-concurrent fashion, which usually means in only one thread at a time.

– from https://docs.sqlalchemy.org/en/13/orm/session_basics.html#is-the-session-thread-safe

In practice, nothing should go wrong until the application is handling multiple things at once (requests, scheduled jobs, ...), and since there are fairly few users and those users are unlikely to be working at the same time as the scheduled jobs (midnight UTC), it's unlikely this will actually cause any problems. However: if anything does go wrong because of this, it will be an absolute nightmare to work out what's happened.

Fixing this will probably involve using SQLAlchemy's scoped_session. One way would be to make the Database store a custom subclass of scoped_session instead of sessionmaker. The subclass would be very similar to SQLAlchemy's implementation, except it would use a new ContextVarsRegistry instead of SQLAlchemy's ScopedRegistry or ThreadLocalRegistry; this new registry would use Python 3.7's contextvars module (supported by aiohttp) to store one session per request. An aiohttp middleware to remove the session at the end of each request will also be necessary.

sersorrel added a commit that referenced this issue Sep 6, 2019
This is a first step towards using one session per async context rather
than sharing a single session throughout the entire application.

See #19.
piyushahuja pushed a commit that referenced this issue Feb 6, 2020
To have one SQLAlchemy session per request, added two middlewares for creating and removing a session. The SQLAlchemy API does not require interacting with the session object directly; instead we use calls to Session class to indirectly call methods on session objects. The first time during the request cycle Session is called, it creates a new session object for the request context (supported by AIOHTTP)  and saves it in ContextLocalRegistry using the contextvars module. Every subsequent call to Session in the request-context lifecycle uses this session object (obtained with a context.get() call)

Resolves: Issue #19
piyushahuja pushed a commit that referenced this issue Feb 7, 2020
To have one SQLAlchemy session per request, added two middlewares for creating and removing a session. The SQLAlchemy API does not require interacting with the session object directly; instead we use calls to Session class to indirectly call methods on session objects. The first time during the request cycle Session is called, it creates a new session object for the request context (supported by AIOHTTP)  and saves it in ContextLocalRegistry using the contextvars module. Every subsequent call to Session in the request-context lifecycle uses this session object (obtained with a context.get() call)

Resolves: Issue #19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant