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

Assertion error when using transaction decorator #123

Closed
SebasAren opened this issue Jul 9, 2019 · 4 comments
Closed

Assertion error when using transaction decorator #123

SebasAren opened this issue Jul 9, 2019 · 4 comments

Comments

@SebasAren
Copy link

I'm using fastapi in combination with this library but when I use the transaction decorator I'm sporadically getting the following assertion error:

File ".../lib/python3.7/site-packages/databases/core.py", line 305, in commit
    assert self._connection._transaction_stack[-1] is self

I'm using a pretty basic fastapi setup making use of the on event startup hook to connect the database. The database is mysql, which i'm connecting to with the aiomysql package (and this library).

I've decorated all of my routes in the following manner :

@app.get("/", response_model=DataModel)
@database.transaction()
async def hello_world(q: str):
    query = sqlalchemy.select(table)
    result = await database.fetch_all(query)
    return result

This works most of the time, but once in a while it raises the above exception. If I remove the transaction decorator everything works as expected.

Why do I get this error? Am I using the decorator improperly?

@tomchristie
Copy link
Member

Not sure - could be to do with the interfaction between the two decorators maybe?
Have you tried them the other way around?
Are there any issues on FastAPI's GitHub repo related to this?
Are there example examples in FastAPI of using it together with databases?

@SebasAren
Copy link
Author

Not sure - could be to do with the interfaction between the two decorators maybe?

This has crossed my mind and seems the most logical explanation

Have you tried them the other way around?

That seems to me to be the "wrong" way around. But I might try it out.

Are there any issues on FastAPI's GitHub repo related to this?

Not that I could find.

Are there example examples in FastAPI of using it together with databases?

The main fastapi documention has an example to set it up, but doesn't use transactions and is pretty basic overall.

While typing this I got another idea of how I could set this up. Using a middleware to attach a transaction to each route, which sounds a lot less error prone. I'm afraid the problem is because of the double decorator.

@circlingthesun
Copy link
Contributor

This took me a while to figure out. I used locust to create some load tests. This issue only happens when two concurrent requests happen on the same endpoint. Calling database.transaction() creates a Transaction object which is then used to decorate a route function. This instance of the Transaction object's state is then shared between all invocations of the route function.

When a route function is called by fastapi, the transaction wrapper function gets a connection and saves it on the object self._connection = self._connection_callable(). When this happens twice the first connection gets overwritten on the transaction object. What happens then is the current transaction object is pushed to a stack on the current connection object. So the stack size will be 1 on each connection but the first connection is lost. When the route function is done executing, the transaction wrapper tries to commit on the connection associated with the transaction object. Before this commit, an assertion happens to see if the transaction object is at the top of the stack. This will be true for the first invocation. The transaction object then gets popped off the stack. When the 2nd invocation commits, the stack will be empty and the assertion fails.

So decorating functions with @database.transaction() that can be run concurrently is a terrible idea because of this shared state. It might be a good idea to deprecate the decorator given this flaw, perhaps someone can think of a fix?

I've created this decorator to work around this:

from functools import wraps

def transaction(func):
    @wraps(func)
    async def wrapper(*args, **kwargs):
        async with database.transaction():
            return await func(*args, **kwargs)

    return wrapper

@zevisert
Copy link
Contributor

I'm relatively certain should be fixed by #546, there's now plenty of discussion in that PR, and in many of the related issues as well if you're still curious. When that's eventually released please let us know if you still experience this error - thank you for reporting!

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

5 participants