AsyncIO's wait_for can hide cancellation in a rare race condition

@ods

BPO	42130
Nosy	@ods, @asvetlov, @cjerdonek, @1st1, @tvoinarovskyi, @Dreamsorcerer, @aaliddell, @crowbarz, @WJCFerguson
PRs	bpo-42130: Fix swallowing of cancellation by wait_for #26097 bpo-42130: Fix for explicit suppressing of cancellations in wait_for() #28149

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = None
created_at = <Date 2020-10-23.16:10:28.990>
labels = ['type-bug', '3.8', '3.9', '3.10', 'expert-asyncio']
title = "AsyncIO's wait_for can hide cancellation in a rare race condition"
updated_at = <Date 2021-12-02.09:55:13.171>
user = 'https://github.com/tvoinarovskyi'

bugs.python.org fields:

activity = <Date 2021-12-02.09:55:13.171>
actor = 'dreamsorcerer'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['asyncio']
creation = <Date 2020-10-23.16:10:28.990>
creator = 'tvoinarovskyi'
dependencies = []
files = []
hgrepos = []
issue_num = 42130
keywords = ['patch']
message_count = 6.0
messages = ['379454', '379541', '379577', '393336', '407489', '407521']
nosy_count = 10.0
nosy_names = ['ods', 'asvetlov', 'chris.jerdonek', 'yselivanov', 'tvoinarovskyi', 'dreamsorcerer', 'aaliddell', 'nmatravolgyi', 'crowbarz', 'jamesf']
pr_nums = ['26097', '28149']
priority = 'normal'
resolution = None
stage = 'patch review'
status = 'open'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue42130'
versions = ['Python 3.8', 'Python 3.9', 'Python 3.10']

Linked PRs

GH-86296: Fix for asyncio.wait_for() swallowing cancellation, and add tests #98607

I have just recently found out that one of my libraries had always been affected by this bug.

Here's a script that reliably reproduces the bug on Python 3.9 - 3.11, without relying on timing coincidence:

import asyncio


async def left(tasks: list[asyncio.Task], event: asyncio.Event):
    me = asyncio.current_task()
    assert tasks[0] is me
    await asyncio.sleep(0.1)
    event.set()
    tasks[1].cancel()
    print('cancelled right')


async def right(tasks: list[asyncio.Task], event: asyncio.Event, use_wait_for: bool):
    try:
        me = asyncio.current_task()
        assert tasks[1] is me
        if use_wait_for:
            await asyncio.wait_for(event.wait(), 1)
        else:
            await event.wait()
        print('right done, was not cancelled')
    except asyncio.CancelledError:
        print('right got cancelled')
        raise


async def left_right(use_wait_for: bool):
    tasks = []
    event = asyncio.Event()
    tasks.append(asyncio.create_task(left(tasks, event)))
    tasks.append(asyncio.create_task(right(tasks, event, use_wait_for)))
    await asyncio.wait(tasks)


if __name__ == '__main__':
    print('Not using wait_for():')
    asyncio.run(left_right(False))
    print('\nUsing wait_for():')
    asyncio.run(left_right(True))

Actual output:

Not using wait_for():
cancelled right
right got cancelled

Using wait_for():
cancelled right
right done, was not cancelled

Using asyncio.timeout instead of asyncio.wait_for fixes the problem for me.

Wow, this has certainly been reported enough times -- a fix was even reported but apparently it's still not fixed.

My head dazzles trying to understand the code of wait_for(). This may have to wait until we have more asyncio experts -- it's too late to fix in 3.11.0 anyways, though possibly we could fix it for 3.11.1 (if there's a PR).

Add tests for happy eyeballs and its internal workings.

@twisteroidambassador

@twisteroidambassador If you want a fix, can you first try @graingert's suggestion from #86296 (comment)? If that works, it'd be great!

Yes, as far as I can see replacing wait_for with timeout solves the issue.

On Python < 3.11 I think using wait instead of timeout is a solution. See #98431.

@gvanrossum

@gvanrossum said:

I wonder if the key problem here isn't the one that caused so much trouble for the Semaphore class as well -- you can await a future, and catch CancelledError, but that doesn't mean the future was cancelled, it could mean the current task was cancelled after the future already completed. Maybe there's still a path through the code that doesn't account for that, somehow?

I think this is the crux of the issue, and an important decision to make for the behavior of wait_for:

When wait_for is being cancelled from the outside, while simultaneously the inner fut is done, should wait_for:

swallow the cancellation and return fut's result / exception, or
accept the cancellation, and thus swallow fut's result / exception?

The current code's behavior is 1: See Lines 470 - 472 (EDIT: and I believe it's intentional, since nothing could be cancelling waiter, so this CancelledError must come from the outside)

cpython/Lib/asyncio/tasks.py

Lines 466 to 479 in ed827d5

    
           try: 
        
               # wait until the future completes or the timeout 
        
               try: 
        
                   await waiter 
        
               except exceptions.CancelledError: 
        
                   if fut.done(): 
        
                       return fut.result() 
        
                   else: 
        
                       fut.remove_done_callback(cb) 
        
                       # We must ensure that the task is not running 
        
                       # after wait_for() returns. 
        
                       # See https://bugs.python.org/issue32751 
        
                       await _cancel_and_wait(fut, loop=loop) 
        
                       raise

while my PR #98431 does 2.

A much smaller-scale fix can also achieve behavior 2 above: #98432

@grangert

I've been lying awake thinking this through. In regards to your question about what should happen when wait_for() is cancelled from the outside, (1) or (2), may I propose

If the result is an exception, raise it; if the result is a "successful" return, re-raise the CancelledError.

My reason is derived from what @grangert convinced me was the correct outcome in some other cancellation edge case (I think it was for either TaskGroup or timeout()). His point was that if a coroutine has an except CancelledError clause but then hits a bug in its exception handling code, thus raising a new exception, that exception should not be suppressed, since it may be the only indication that there's a bug in the app. OTOH if the exception handling for CancelledError merely returns (instead of re-raising it) then we should continue propagating the cancellation outward (where it may be expected by some other code).

But wait, there's more! In 3.11 there's another wrinkle, uncancel(). When you call task.cancel() and it returns True, a counter on the task is incremented. (And it may take several event loop iterations before the task actually completes.) Later when inspecting the result of the task, you are supposed to call task.uncancel() to decrement that counter again. Technically this is even the right thing to do if the task's state is not "cancelled", but only if your earlier task.cancel() call returned True.

The cancel-uncancel dance is intended to make it possibly to reliably distinguish between an external cancellation and a cancellation you initiated, even if both occurred simultaneously on the same object. You have to set a flag that remembers the result of your cancel() call, and later, when the task is done, if that flag is set, you call uncancel(), and if it returns zero the cancellation was yours, otherwise it was external.

I have a feeling that much of the complexity in wait_for() is because it was trying to work around the lack of such a mechanism.

With all that, I still haven't fully grokked either of your proposed solutions, for which I apologize (all the above was just mental preparation). I also notice that you haven't added a new test yet that demonstrates the problem.

Finally, when I try the naive version of @graingert's ideas use timeout(), I get failing tests in test_waitfor.py, so either the tests are over-constraining a certain undesirable behavior, or there's a bug in timeout(), or wait_for() has some historical behavior that is just different but which we would need to preserve.

@grangert

If the result is an exception, raise it; if the result is a "successful" return, re-raise the CancelledError.

My reason is derived from what @grangert convinced me was the correct outcome in some other cancellation edge case (I think it was for either TaskGroup or timeout()). His point was that if a coroutine has an except CancelledError clause but then hits a bug in its exception handling code, thus raising a new exception, that exception should not be suppressed, since it may be the only indication that there's a bug in the app. OTOH if the exception handling for CancelledError merely returns (instead of re-raising it) then we should continue propagating the cancellation outward (where it may be expected by some other code).

I like this proposal. It avoids the most unexpected behavior of (1), in which wait_for is cancelled from the outside, but then it returns normally like the cancellation never happened. And it gives the programmer a bit more information than (2).

If we go with this, then I think we have a complete specification for wait_for.

There are 3 things that can happen once wait_for has been entered: being cancelled externally, exceeding the timeout, or the inner awaitable becomes done. Let's assume all 3 could be happening simultaneously.

Existing comments in wait_for describes expected behavior:

We must ensure that the task is not running after wait_for() returns. See https://bugs.python.org/issue32751

In case task cancellation failed with some exception, we should re-raise it. See https://bugs.python.org/issue40607

The proposed behavior is:

If being cancelled externally:
- If awaitable is done:
  - if awaitable is cancelled or has an exception, raise it
  - otherwise, raise the external cancellation
- Otherwise (awaitable is not done), cancel it and wait until it's done, then do the above
  - (Does it matter if awaitable's cancellation is ours?)
- (Note that whether timeout is exceeded does not factor into these decisions)
If timeout exceeded:
- If awaitable is done, return / raise its result
- Otherwise (awaitable is not done), cancel it and wait until it's done
  - If awaitable is cancelled and the cancellation is ours, raise TimeoutError
  - Otherwise, return / raise awaitable's result
awaitable is done. Return / raise its result

Does this look reasonable?

But wait, there's more! In 3.11 there's another wrinkle, uncancel(). When you call task.cancel() and it returns True, a counter on the task is incremented. (And it may take several event loop iterations before the task actually completes.) Later when inspecting the result of the task, you are supposed to call task.uncancel() to decrement that counter again. Technically this is even the right thing to do if the task's state is not "cancelled", but only if your earlier task.cancel() call returned True.

The cancel-uncancel dance is intended to make it possibly to reliably distinguish between an external cancellation and a cancellation you initiated, even if both occurred simultaneously on the same object. You have to set a flag that remembers the result of your cancel() call, and later, when the task is done, if that flag is set, you call uncancel(), and if it returns zero the cancellation was yours, otherwise it was external.

I wonder whether the following sequence of events is possible:

A calls some_task.cancel()
B calls some_task.cancel()
A calls some_task.uncancel(), and decides it's not responsible for the cancellation
B calls some_task.uncancel() and decides it's responsible for the cancellation

If it's possible, does it matter?

Your proposal looks reasonable. There is some devil is in the details. How can we tell we're being cancelled externally? I suppose the current wait_for() code can tell because it has an outer future and an inner task, and we consider only cancellation of the outer future to be external. I sort of wish we didn't have to have both a future and a task, but in the end timeout() uses that too, so I should just be okay with that.

Re: the sequence of cance/uncancel calls, it's certain that we may not be able to tell whether A or B cancelled first, but we do know in which order they are handled, and the convention is that whoever handles last should get to decide. Everyone who handles prior to that will see uncancel() return > 0, and by convention has to propagate the outer cancellation.

There are still some odd corner cases though, e.g. when both A and B cancel, then the coroutine catches the cancellation and returns None. The first handler (typically the last one to cancel, so B) ought to still call uncancel(), will find it > 0, and then what? Raise a fresh CancelledError? Or do whatever it normally does when the coroutine returns None?

FWIW on how to proceed, let's fix it in a way that's compatible with 3.10 first (so no uncancel() calls), and then in a later PR get the uncancel() calls in, possibly by using timeout(), e.g. as in Kumar's PR (#98518).

PS. A final wrinkle with uncancel is that it may not exist -- Future doesn't define it, and 3rd party event loops may create tasks that don't have it yet.

While writing tests for the proposed behavior, I realized that it will make await wait_for(fut, timeout=None) and await fut behave differently. If fut becomes done at the same time await fut is cancelled, the await will always raise CancelledError no matter what result / exception fut has, while the proposed wait_for() will act differently depending on fut.

This also means that to pass these tests, the "easy path" of wait_for() where timeout is None cannot be a simple return await fut anymore.

Does this still sound like a good idea?

Hmm, that does sound like a problem. I've been see-sawing on this quite a bit. At this point I believe Kumar's PR is the gold standard for behavior, and it follows what await fut does.

(Note that I'm currently severely backed up and it may take a few days to dig myself out, until then I'm not sure you should believe anything I say.)

I'll write down a new set of proposed behavior at the bottom. These will always raise CancelledError when cancelled externally, discarding any result / exception of the inner awaitable if any.

This makes await wait_for(fut, timeout=None) and await fut behaves the same, and is also the minimal change from existing behavior that fixes the current bug.

I have also changed my latest PR to match. If we end up preferring the previous behavior, we can always revert the one commit in the PR.

New proposed behavior (changes are italicized):

If being cancelled externally:
- If awaitable is done: continue with the external cancellation (discarding awaitable's result / exception if any)
- Otherwise (awaitable is not done), cancel it and wait until it's done, then do the above
If timeout exceeded:
- If awaitable is done, return / raise its result
- Otherwise (awaitable is not done), cancel it and wait until it's done
  - If awaitable is cancelled and the cancellation is ours, raise TimeoutError
  - Otherwise, return / raise awaitable's result
awaitable is done. Return / raise its result

I believe using asyncio.shield() can offer a work around to this in at least Python 3.8.6 (I've only tested it in this version). Modifying the test script that reproduces the issue given here as follows (wrapping the coroutine in the wait_for() call in asyncio.shield()):

import asyncio
from typing import List


async def left(tasks: List[asyncio.Task], event: asyncio.Event):
    me = asyncio.current_task()
    assert tasks[0] is me
    await asyncio.sleep(0.1)
    event.set()
    tasks[1].cancel()
    print('cancelled right')


async def right(tasks: List[asyncio.Task], event: asyncio.Event, use_wait_for: bool):
    try:
        me = asyncio.current_task()
        assert tasks[1] is me
        if use_wait_for:
            await asyncio.wait_for(asyncio.shield(event.wait()), 1)
        else:
            await event.wait()
        print('right done, was not cancelled')
    except asyncio.CancelledError:
        print('right got cancelled')
        raise


async def left_right(use_wait_for: bool):
    tasks = []
    event = asyncio.Event()
    tasks.append(asyncio.create_task(left(tasks, event)))
    tasks.append(asyncio.create_task(right(tasks, event, use_wait_for)))
    await asyncio.wait(tasks)


if __name__ == '__main__':
    print('Not using wait_for():')
    asyncio.run(left_right(False))
    print('\nUsing wait_for():')
    asyncio.run(left_right(True))

Gives the desired output:

Not using wait_for():
cancelled right
right got cancelled

Using wait_for():
cancelled right
right got cancelled

This suggests the issue is to do with the exception being swallowed somewhere deep in the call stack.

Add a non-random test to test_stagger.py.

And fixed in 3.12 by gh-96764. (We're not 100.00% sure that the fix doesn't disturb some workaround, so we're not backporting the fix to 3.11 even though that has timeout(), and we would have to devise a totally different fix for 3.10, which we're not inclined to do, sorry. But going forward it should be fixed.

asyncio.wait_for() is too buggy. use util.wait_for2() instead

Add a non-random test to test_stagger.py.

for feedback I just wasted several partial days tracking a production only issue that only happens once a day almost certainly due to this (add logging, narrow down, add more logging..etc...etc). I'd reconsider the decision as it can waste many development hours trying to find what the issue is when the code at hand seems reasonable, until you come to the conclusion that the library must be swallowing the exception. Either that or at a minimum update the docs to say don't use it in python<3.12 or there be dragons.

Phase E: examples, demo #4 (bpo-42130 reproduced), launch draft, publ…

Propagate external cancellation through worker waits on Python 3.10 a…

asyncio.wait_for() is too buggy. use util.wait_for2() instead

fix(ws): Broadcasts erreichen alle Prozesse, nur noch ein Primary Wor…

[Lint] Flag asyncio.wait_for with a pylint check; use the cancel-safe…

Navigation Menu

Repository navigation

AsyncIO's wait_for can hide cancellation in a rare race condition #86296

Linked PRs

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

	try:
	# wait until the future completes or the timeout
	try:
	await waiter
	except exceptions.CancelledError:
	if fut.done():
	return fut.result()
	else:
	fut.remove_done_callback(cb)
	# We must ensure that the task is not running
	# after wait_for() returns.
	# See https://bugs.python.org/issue32751
	await _cancel_and_wait(fut, loop=loop)
	raise

AsyncIO's wait_for can hide cancellation in a rare race condition #86296

Description

Linked PRs

Activity

twisteroidambassador commented on Oct 17, 2022

gvanrossum commented on Oct 18, 2022

gvanrossum commented on Oct 18, 2022

twisteroidambassador commented on Oct 18, 2022

twisteroidambassador commented on Oct 19, 2022

twisteroidambassador commented on Oct 19, 2022

twisteroidambassador commented on Oct 19, 2022

gvanrossum commented on Oct 20, 2022

twisteroidambassador commented on Oct 21, 2022

gvanrossum commented on Oct 21, 2022

twisteroidambassador commented on Oct 24, 2022

gvanrossum commented on Oct 25, 2022

twisteroidambassador commented on Oct 26, 2022

laker-93 commented on Oct 28, 2022

gvanrossum commented on May 24, 2023

thehesiod commented on Dec 13, 2024

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions