-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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 bug with async_lock
implementation
#2695
Conversation
87c5900
to
8a7af21
Compare
inner.cancel() | ||
|
||
outer = asyncio.create_task(_utilize_async_lock()) | ||
await outer |
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.
It might be better to replace this line with await asyncio.wait_for(outer, 2)
which will raise an asyncio.TimeoutError
if outer takes more than 2 more seconds to execute. That way we'll get a failed test rather than a hang if there's a future bug with the lock.
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.
👍🏼 good point. I actually tweaked the timeouts quite a bit to make the test faster in general.
fe78025
to
dc84582
Compare
- ``async_lock`` would hold onto the lock even if a task had been cancelled. This change ensures that the lock will be released if an exception is thrown during ``loop.run_in_executor()``.
dc84582
to
3ecd6e7
Compare
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.
lgtm!
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.
LGTM! 🚢
What was wrong?
closes #2693
How was it fixed?
async_lock
if something goes wrong during a task.Todo:
Cute Animal Picture