-
Notifications
You must be signed in to change notification settings - Fork 262
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
Using ContextVar to hold connections does not guarantee they are always unique to tasks. #230
Comments
This is (almost) a duplicate of #176 I posted some workarounds there. I am opening new connections in my prod and that works very well; my opinion is that this ContextVar feature is harmful and should be removed. It does not save much performance (I py-spy regularly) and leads to many problems, some of them are performance-critical. |
@aminalaee unfortunately this does not fix this issue. The sample code above still fails. The problem is still that although the code comments in Unfortunately I don't think I have permission to re-open this issue - is that something you can do? |
@trvrm Thanks, I'll take a look into it. |
Thanks for the great write up @trvrm - I agree about how connections should not be inherited to child tasks, but rather new connections should be acquired. In #546, I think I have a solution. Each database instance stores a |
I think there's a fairly fundamental problem with the way that database connections are managed using
ContextVar
.Database.connection()
is designed to return aConnection
object that is unique to an asyncio Task, but because of the way that context is copied during Task creation, this is not always the case.Consider the following code:
This raises an assertion, because when
gather
creates a new task, it copies the current context into the newly created coroutine.This is the fundamental problem behind #134 and possibly several other issues. In general it makes it impossible to safely use this library in any code that uses
gather
orcreate_task
.Unfortunately I'm not sure what the best way of fixing this is. The only idea I've had so far is storing the connection as an attribute of
asyncio.current_task()
rather than in a ContextVar, but that feels a bit hacky.The text was updated successfully, but these errors were encountered: