Repository navigation
asyncio.timeout(0) and asyncio.timeout(-1) cancellation is delayed #95051
Description
Bug report
I expect this to raise a TimeoutError:
async def amain():
async with asyncio.timeout(-11):
await asyncio.sleep(0)
asyncio.run(amain())Instead I have to add an extra asyncio.sleep(0) to get a TimeoutError
async def amain():
async with asyncio.timeout(-11):
await asyncio.sleep(0) # does not raise CancelledError
await asyncio.sleep(0) # raises CancelledError
asyncio.run(amain())Your environment
- CPython versions tested on:
- Operating system and architecture:
Activity
graingert commentedon Jul 20, 2022
This also makes refactoring
asyncio.wait_forto useasyncio.timeoutdifficult: #31847 (comment) difficultTinche commentedon Jul 21, 2022
Sorry, deleted my last comment since it was wrong. I'm going to try to dive into this a little to get some understanding.
Tinche commentedon Jul 21, 2022
This is a race condition. Two things need to happen on the same event loop iteration, but it turns out A happens before B, and not B before A. I don't think asyncio has a spec anywhere saying this is wrong, so a legitimate response might be to just live with it.
Maybe it's confusing to users though so we want to do something about it.
The event loop maintains two callback lists:
self._ready- these are callbacks ready to be run right now. (actually a deque)self._scheduled- these are callbacks that will be ready at a certain time t. t might be in the past. (a list, but actually a heapq)When processing a loop step, the event loop looks at
_scheduledfirst, taking all callbacks that are due to run and appending them to_ready. So callbacks from_scheduledwill run after the callbacks in_ready. Then it executes all callbacks in_ready.timeoutusesloop.call_atunder the hood, andcall_atusesself._scheduled.sleephas a special case for0and negative values; this case usesloop.call_soon, andcall_soonusesself._ready.So this explains the issue:
amain()starts runningtimeoutputs a callback intoloop._scheduledsleepstopsamain()and puts a callback intoloop._readytimeoutcallback gets appended toloop._ready, but the sleep callback is firstsleepcallback runs, drives the coroutine to completiontimeoutcallback runs, the coroutine/task is doneA couple of proposals how to handle this:
A. do nothing, leave it up to the event loop how to resolve cases like this
B. change
timeoutto special case delays<= 0to useloop.call_sooninstead ofloop.call_at. This would make the timeout callback go first, but the fix feels narrowly scoped, might leave the race condition still to happen under a different timeout.C. special case
loop.call_atto internally callloop._call_soonif the deadline is in the past.D. change how the event loop processes callbacks. The scheduled callbacks could be prepended to
self._readyinstead of appended. This would again make the timeout callback go first. Also feels more intuitive to me as far as callback scheduling goes. I guess it'd make the event loop do slightly more work. Might also be backwards-incompatible, but it doesn't matter since it's unspecified?My personal preference might be D, and then A.
@gvanrossum I know you've been in this space recently, would appreciate your feedback.
graingert commentedon Jul 21, 2022
This is the uvloop behavior
graingert commentedon Jul 21, 2022
I think it's probably too late in the 3.11 release cycle to touch something this low level in the event loop.
I think tweaking
asyncio.timeoutto match the behavior ofasyncio.wait_for(x, 0)andasyncio.sleep(0)makes the most sense to megvanrossum commentedon Jul 21, 2022
Tin, thanks for the deep dive! I have to agree with Thomas that we don't want to touch the event loop or call_at. But since timeout is new, I'd be okay if we tweaked that (i.e., B). If either of you want to produce a PR that includes a unit test demonstrating the need for a fix, I'd approve it and it might not be hard to convince Pablo to allow it in 3.11, but I think A is also acceptable for 3.11 (race conditions gonna race). For 3.12 we might be able to do C or D.
Fidget-Spinner commentedon Jul 24, 2022
I'm not an asyncio expert, but I don't think we should go with D. Prepending may cause starvation in certain scenarios (the callbacks at the back of the deque may never get to run).
Tinche commentedon Jul 24, 2022
So out of curiosity I checked out what UVLoop does, since in my experience anyone who's serious about asyncio runs it anyway.
They prepend the scheduled callbacks (well, lib uv does under the hood), so they do D. @graingert 's example works expectedly there.
This increases my confidence in that approach, since I'm already using it in production ;)
@Fidget-Spinner Not sure how callbacks at the end of the deque not running is any worse than callbacks in the heapq.
gh-95051: ensure that timeouts scheduled with `asyncio.Timeout` that …
Tinche commentedon Jul 24, 2022
So I did some more digging, especially through libuv (the library that powers NodeJS and uvloop).
As far as I can see, libuv does what I'm proposing. On each iteration of the loop, expired timers (= callbacks with a deadline that is before now) get run first, and then the
_readyset (which they call 'idle handlers').gh-95051: ensure that timeouts scheduled with `asyncio.Timeout` that …
kumaraditya303 commentedon Oct 2, 2022
Fixed by #95109