asyncio.timeout(0) and asyncio.timeout(-1) cancellation is delayed #95051

graingert · 2022-07-20T14:37:15Z

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:

The text was updated successfully, but these errors were encountered:

graingert · 2022-07-20T14:39:35Z

This also makes refactoring asyncio.wait_for to use asyncio.timeout difficult: #31847 (comment) difficult

Tinche · 2022-07-21T01:10:23Z

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 · 2022-07-21T15:19:50Z

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 _scheduled first, taking all callbacks that are due to run and appending them to _ready. So callbacks from _scheduled will run after the callbacks in _ready. Then it executes all callbacks in _ready.

timeout uses loop.call_at under the hood, and call_at uses self._scheduled.
sleep has a special case for 0 and negative values; this case uses loop.call_soon, and call_soon uses self._ready.

So this explains the issue:

loop iteration starts, amain() starts running
timeout puts a callback into loop._scheduled
sleep stops amain() and puts a callback into loop._ready
Another loop iteration starts
timeout callback gets appended to loop._ready, but the sleep callback is first
sleep callback runs, drives the coroutine to completion
no timeout exception, since when the timeout callback runs, the coroutine/task is done

A 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 timeout to special case delays <= 0 to use loop.call_soon instead of loop.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_at to internally call loop._call_soon if the deadline is in the past.
D. change how the event loop processes callbacks. The scheduled callbacks could be prepended to self._ready instead 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 · 2022-07-21T16:08:00Z

C. special case loop.call_at to internally call loop._call_soon if the deadline is in the past.

This is the uvloop behavior

graingert · 2022-07-21T16:19:25Z

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.timeout to match the behavior of asyncio.wa 8000 it_for(x, 0) and asyncio.sleep(0) makes the most sense to me

gvanrossum · 2022-07-21T22:26:12Z

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 · 2022-07-24T05:17:07Z

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 · 2022-07-24T18:42:43Z

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.

…have already expired are deliverered promptly (#95109) Co-authored-by: Kumar Aditya <59607654+kumaraditya303@users.noreply.github.com>

… that have already expired are deliverered promptly (pythonGH-95109) Co-authored-by: Kumar Aditya <59607654+kumaraditya303@users.noreply.github.com> (cherry picked from commit 0c6f898) Co-authored-by: Thomas Grainger <tagrain@gmail.com>

Tinche · 2022-07-24T23:55:50Z

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 _ready set (which they call 'idle handlers').

…have already expired are deliverered promptly (GH-95109) (GH-95216) Co-authored-by: Kumar Aditya <59607654+kumaraditya303@users.noreply.github.com> (cherry picked from commit 0c6f898) Co-authored-by: Thomas Grainger <tagrain@gmail.com>

kumaraditya303 · 2022-10-02T07:03:34Z

Fixed by #95109

graingert added the type-bug An unexpected behavior, bug, or error label Jul 20, 2022

This was referenced Jul 20, 2022

quattro.fail_after(0) and and quattro.fail_after(-1) cancellation is delayed Tinche/quattro#2

Closed

bpo-42130: Fix swallowing of cancellation by wait_for #26097

Closed

gvanrossum added topic-asyncio 3.11 only security fixes 3.12 only security fixes labels Jul 21, 2022

bedevere-bot mentioned this issue Jul 21, 2022

gh-95051: ensure that timeouts scheduled with asyncio.Timeout that have already expired are deliverered promptly #95109

Merged

graingert added a commit to graingert/cpython that referenced this issue Jul 21, 2022

pythongh-95051: handle timeouts.timeout(0) and timeouts.timeout(-1)

dce6d74

gvanrossum pushed a commit that referenced this issue Jul 24, 2022

gh-95051: ensure that timeouts scheduled with asyncio.Timeout that …

0c6f898

…have already expired are deliverered promptly (#95109) Co-authored-by: Kumar Aditya <59607654+kumaraditya303@users.noreply.github.com>

bedevere-bot mentioned this issue Jul 24, 2022

[3.11] gh-95051: ensure that timeouts scheduled with asyncio.Timeout that have already expired are deliverered promptly (GH-95109) #95216

Merged

kumaraditya303 closed this as completed Oct 2, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

asyncio.timeout(0) and asyncio.timeout(-1) cancellation is delayed #95051

asyncio.timeout(0) and asyncio.timeout(-1) cancellation is delayed #95051

asyncio.timeout(0) and asyncio.timeout(-1) cancellation is delayed #95051

asyncio.timeout(0) and asyncio.timeout(-1) cancellation is delayed #95051

Comments