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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix issue with concurrent contextual stack modification #747
Conversation
Thanks for the very detailed explanation and the PR, and sorry for the trouble! It does make sense to me - I'd like to further check the code, at the same time would you mind fixing the syntax for Python 3.5 pls? |
54d6063
to
4c928cb
Compare
I have fixed python 3.5 syntax error. Sorry about that, I forget that |
Hi @fantix, |
Great work @uriyyo I was having the same issue over similar code steps and your PR fixed it. |
Oh sorry for the late reply - I'll take a deeper look tonight. |
Hi @fantix Any updates? Had you a chance to take a look at this PR? |
He's probably been busy. While I do understand the issue, I ponder if there is an easier way to fix it, as linked list seems a bit overkill in this case. |
Oh thank you Tony! Yeah I'm sorry that I've been on a move and trying to put up a new uvloop release. |
While this PR can fix the case in the test, the stack is still shared with tasks created by current coroutine. Ideally, in my mind, a new coroutine can automatically reset contextvars to empty. But it's not how contextvars works. Instead the new coroutine inherits the current one. The solution I can think of now is to create task with _ctx reset to None before the actual execution. Gino can probably expose a method to do that. Any thought? (Another option is to use |
06d3761
to
10e8b2c
Compare
I have reimplemented this PR. I agree that the previous solution with a linked-list can be overkill for this issue. With the new approach contextual stack is bound to task where it was created, in a case when ctx is using in another task then ctx will be rewritten.
This issue also resolved in this PR. What is your opinion regarding the new implementation? |
Looks good. One concern is if we add support to trio later, we need to apply similar strategy there. BTW, can you rebase on latest master, which fixed CI? |
10e8b2c
to
c43f8ee
Compare
Hi Tony, I have rebased on master. Thanks for feedback regarding implementation馃檪 |
Thanks for the fix! |
We are using
gino
in production, and we had a lot of unexpected exceptions fromasynpg
with a message:The problem was, that these exceptions were unpredictable and we had no idea what we a doing wrong.
Basically, we a using
asyncio
based web framework and we had a lot of code patterns like this:And sometimes we had an exception at this place:
This exception cancels one of the running worker tasks.
Today after hours of debugging I have found the root cause of this issue馃コ
The problem was with the concurrent
_ContextualStack._ctx
value modification. When we are creating a newasyncio
task we copy currentcontextvars
to the created task. In a case when there are reusable connections it will copydeque
of those connections to the context of a child task. In a case when we acquire a new DB connection in a child task it will modify the deque of reusable connection which is a completely samedeque
that are using atmain
task.I hope you understood the issue)
The solution that I used is to use an immutable single linked list as a contextual stack, in such case concurrent tasks won't mutate the stack of each other.
@fantix It will be great for me and my team if you can review this PR and release new version of a
gino
which will include this fix, so we can remove the workaround from our code that we currently have.