-
-
Notifications
You must be signed in to change notification settings - Fork 31k
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
gh-129195: use future_add_to_awaited_by/future_discard_from_awaited_by in asyncio.staggered.staggered_race #129253
gh-129195: use future_add_to_awaited_by/future_discard_from_awaited_by in asyncio.staggered.staggered_race #129253
Conversation
graingert
commented
Jan 24, 2025
•
edited by bedevere-app
bot
Loading
edited by bedevere-app
bot
- Issue: asyncio staggered race missing calls to asyncio.future_add_to_awaited_by() and asyncio.future_discard_from_awaited_by() #129195
…ited_by in staggered race
ddeeee9
to
7626ae4
Compare
@@ -63,6 +64,7 @@ async def staggered_race(coro_fns, delay, *, loop=None): | |||
""" | |||
# TODO: when we have aiter() and anext(), allow async iterables in coro_fns. | |||
loop = loop or events.get_running_loop() | |||
parent_task = tasks.current_task(loop) |
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.
What happens if we're not running in a task? Should we assert that this is non-None?
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.
one possible outcome is that there's no task or running loop:
import asyncio.staggered
async def main():
async def asyncfn():
pass
await asyncio.staggered.staggered_race([asyncfn, asyncfn], delay=None)
main().__await__().send(None)
we get a nice traceback, saying that:
./python demo.py
Traceback (most recent call last):
File "/home/graingert/projects/cpython/demo.py", line 9, in <module>
main().__await__().send(None)
~~~~~~~~~~~~~~~~~~~~~~~^^^^^^
File "/home/graingert/projects/cpython/demo.py", line 7, in main
await asyncio.staggered.staggered_race([asyncfn, asyncfn], delay=None)
File "/home/graingert/projects/cpython/Lib/asyncio/staggered.py", line 66, in staggered_race
loop = loop or events.get_running_loop()
~~~~~~~~~~~~~~~~~~~~~~~^^
RuntimeError: no running event loop
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.
if you manually pass a loop, and step the coroutine outside of a task:
import asyncio.staggered
async def main(loop):
async def asyncfn():
print("hello")
await asyncio.staggered.staggered_race([asyncfn, asyncfn], delay=None, loop=loop)
return "world"
loop = asyncio.EventLoop()
coro = main(loop).__await__()
f = coro.send(None)
loop.run_until_complete(f)
try:
coro.send(None)
except StopIteration as e:
print(e.value)
by some miracle it all works, this is because future_add_to_awaited_by and future_discard_from_awaited_by is noop if any arg is not a future (eg is None) and we don't have any further use of the parent task
./python demo.py
hello
world
so we should probably leave it like that.
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.
I don't think it's worth testing this usecase though
@@ -148,6 +152,7 @@ async def run_one_coro(ok_to_start, previous_failed) -> None: | |||
try: |
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.
Above this--for cancelling all the coroutines, future_discard_from_awaited_by
should get called, right?
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 add_done_callback callbacks are called on cancellation, so it will get called
Misc/NEWS.d/next/Library/2025-01-24-10-48-32.gh-issue-129195.89d5NU.rst
Outdated
Show resolved
Hide resolved
…9d5NU.rst Co-authored-by: Kumar Aditya <[email protected]>
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 as well.