-
Notifications
You must be signed in to change notification settings - Fork 9
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
fix: Async auth lock #359
fix: Async auth lock #359
Conversation
I want to add tests before marking it ready for review again. |
finally: | ||
# token gets updated only after flow.send is called | ||
# so unlock only after that | ||
if self._lock.locked() and self._lock._owner_task == get_current_task(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there situations where we end up here, but the task that has acquired the lock if different?
In case no, can we still use context manager?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I remember reproducing it when the lock was still held but another task has reached this point. I don't remember specifics anymore (it's been a while) but I think there is a point when the token has been updated but the lock is not yet released so another thread progresses up until this point by skipping lock on L152.
Using a context manager is not really possible as we want to lock it only when doing the first auth. However, async_auth_flow
is called on every request so we want to avoid always locking like we did before, since it leads to synchronous execution.
Quality Gate passedIssues Measures |
FIR-30447
More intelligent auth lock that doesn't lock on each request, but only when auth flow is executed.
Added tests that verify that we're truly async.