-
Notifications
You must be signed in to change notification settings - Fork 228
Migrate work for finished CurrentThreadExecutor
to previous executor
#494
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
Migrate work for finished CurrentThreadExecutor
to previous executor
#494
Conversation
1cd49b6
to
8a2717c
Compare
8a2717c
to
2a7ba5e
Compare
A CurrentThreadExecutor may terminate with work still remaining in its queue, or new work may be submitted later. We previously discarded remaining work, leading to deadlocks, and raised an error on submitting late work. Instead, execute remaining work immediately, and if there’s another CurrentThreadExecutor for the same thread below us on the stack, redirect late work there to allow it to eventually run. Doing this in a thread-safe way requires replacing the queue.Queue abstraction with collections.deque and threading.ConditionVariable (the same primitives used to implement queue.Queue). Fixes django#491; fixes django#492. Signed-off-by: Anders Kaseorg <[email protected]>
Signed-off-by: Anders Kaseorg <[email protected]>
2a7ba5e
to
3ece1c9
Compare
tests/test_sync.py
Outdated
|
||
|
||
def test_two_nested_tasks_with_asyncio_run() -> None: | ||
barrier = asyncio.Barrier(3) |
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.
We'll need to skip this test on <PY311
. I think that's OK.
I'm really looking forward to see this merged and a new release get out so I can fix the translations issues at my work. Let me know if any of you need any help in making that happening :) e.g. I could add the missing |
Avoids mypy error on asyncio.Barrier API.
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.
Thanks for the work here @andersk — looks good.
I just pushed some edits to get the tests/mypy passing for Python <3.11. Can you confirm if you're happy with those?
👍
Yeah that looks good to me. Thanks! I can look into fixing up the test for older Python later, but we don’t need to block on that. |
@andersk Super. Thanks for confirming. I'll get this in tomorrow now and look for a release by the end of the week. Thanks for the effort, and to @bellini666 too for your review (which got me over the dread-hump 🥳) |
A
CurrentThreadExecutor
may terminate with work still remaining in its queue, or new work may be submitted later. We previously discarded remaining work, leading to deadlocks, and raised an error on submitting late work. But if there’s anotherCurrentThreadExecutor
for the same thread below us on the stack, we should instead migrate the extra work there to allow it to eventually run.Doing this in a thread-safe way requires replacing the
queue.Queue
abstraction withcollections.deque
andthreading.ConditionVariable
(the same primitives used to implementqueue.Queue
).async_to_sync
→sync_to_async
→async_to_sync
→create_task
→sync_to_async
#492CurrentThreadExecutor
reentrant and reusing it #493