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

[twisteroidambassador]

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

[twisteroidambassador]

test_asyncio.test_waitfor is expected to fail with these tests.

In particular, these are the expected failures:

======================================================================
ERROR: test_simultaneous_self_cancel_and_inner_exc (test.test_asyncio.test_waitfor.AsyncioWaitForTest.test_simultaneous_self_cancel_and_inner_exc) (waitfor_timeout=None)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "~\Documents\git\cpython\Lib\test\test_asyncio\test_waitfor.py", line 336, in test_simultaneous_self_cancel_and_inner_exc
    await self.simultaneous_self_cancel_and_inner_result(
  File "~\Documents\git\cpython\Lib\test\test_asyncio\test_waitfor.py", line 323, in simultaneous_self_cancel_and_inner_result
    return await waitfor_task
           ^^^^^^^^^^^^^^^^^^
  File "~\Documents\git\cpython\Lib\asyncio\tasks.py", line 445, in wait_for
    return await fut
           ^^^^^^^^^
asyncio.exceptions.CancelledError

======================================================================
FAIL: test_simultaneous_self_cancel_and_inner_result (test.test_asyncio.test_waitfor.AsyncioWaitForTest.test_simultaneous_self_cancel_and_inner_result) (waitfor_timeout=0.1)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "~\Documents\git\cpython\Lib\test\test_asyncio\test_waitfor.py", line 328, in test_simultaneous_self_cancel_and_inner_result
    with self.assertRaises(asyncio.CancelledError):
AssertionError: CancelledError not raised

======================================================================
FAIL: test_simultaneous_self_cancel_and_inner_result (test.test_asyncio.test_waitfor.AsyncioWaitForTest.test_simultaneous_self_cancel_and_inner_result) (waitfor_timeout=10)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "~\Documents\git\cpython\Lib\test\test_asyncio\test_waitfor.py", line 328, in test_simultaneous_self_cancel_and_inner_result
    with self.assertRaises(asyncio.CancelledError):
AssertionError: CancelledError not raised

----------------------------------------------------------------------

[twisteroidambassador]

The latest commit is enough to fix #86296, however it still fails one of the new tests added.

Specifically, with await wait_for(fut, timeout=None), if wait_for is cancelled simultaneously with fut.set_exception(), the new test requires wait_for to reraise fut's exception, while existing behavior is to raise CancelledError. (Worth noting, the existing behavior means await fut and await wait_for(fut, timeout=None) behaves the same.)

Now the question is: is this over-specification? If so, we should remove this test and call it a day.

[twisteroidambassador]

I have changed wait_for's behavior upon external cancellation. It will always raise CancelledError now.

Also changed new tests to match. Now all tests should pass.

[gvanrossum]

I need to put this back into my review queue!

[robsdedude]

FWIW, there already have been

• bpo-42130: Fix swallowing of cancellation by wait_for #26097
• bpo-42130: Fix for explicit suppressing of cancellations in wait_for() #28149
• GH-96764: rewrite asyncio.wait_for to use asyncio.timeout #98518 (not backportable, uses asyncio.timeout)

trying to tackle this issue.

I'll state it again, I think this is a really nasty bug comparable to a function in the std lib swallowing a KeyboardInterrupt. And it has been around since 3.8. I really hope this gets resolved (and hopefully also backported) in a timely manner.

[netlify]

✅ Deploy Preview for python-cpython-preview canceled.

Name	Link
🔨 Latest commit	02a624c
🔍 Latest deploy log	https://app.netlify.com/sites/python-cpython-preview/deploys/6399ad3ee3d81200085d9b09

[kumaraditya303]

AFAIK, this is superseded by #96764 which fixed the issue and tests have been added for each case. The tests being added here seems much harder to understand than whose which are already added.

[robsdedude]

@kumaraditya303 with the difference that this PR could be ported back to Python 3.10 while the one you linke depends on asyncio.timeout which was only introduced in Python 3.11.

[gvanrossum]

I’m not very keen on having to learn yet again all the edge cases of wait_for. Maybe we can just leave 3.10 alone?

[kumaraditya303]

I’m not very keen on having to learn yet again all the edge cases of wait_for. Maybe we can just leave 3.10 alone?

Nor am I, there is only one bug fix release left for 3.10 and I don't want to risk introducing any issue in a stable branch.

[gvanrossum]

So let's just close this.