◐ Shell
reader mode source ↗
Skip to content

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

Closed
twisteroidambassador wants to merge 7 commits into
python:mainfrom
twisteroidambassador:wait_for_specs
Closed

GH-86296: Fix for asyncio.wait_for() swallowing cancellation, and add tests#98607
twisteroidambassador wants to merge 7 commits into
python:mainfrom
twisteroidambassador:wait_for_specs

Conversation

@twisteroidambassador

@twisteroidambassador twisteroidambassador commented Oct 24, 2022 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Contributor

@bedevere-bot bedevere-bot added awaiting review tests Tests in the Lib/test dir labels Oct 24, 2022
@twisteroidambassador

twisteroidambassador commented Oct 24, 2022

Copy link
Copy Markdown
Contributor Author

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 twisteroidambassador changed the title GH-86296: Add tests for wait_for() behavior when things happen simultaneously. Oct 25, 2022
@twisteroidambassador

twisteroidambassador commented Oct 25, 2022

Copy link
Copy Markdown
Contributor Author

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.

@gvanrossum gvanrossum mentioned this pull request Oct 25, 2022
Merged
@twisteroidambassador
Remove tests for simultaneous *timeout* and *inner becoming done*. …
c834122 
The existing method of setting identical timeout / sleep lengths is not reliable.
@twisteroidambassador
Spec change: when cancelled, always raise CancelledError. …
7660179 
This means swallowing the inner future's result / exception, if they are set simultaneously with the external cancellation.
@twisteroidambassador

twisteroidambassador commented Oct 26, 2022

Copy link
Copy Markdown
Contributor Author

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

gvanrossum commented Nov 30, 2022

Copy link
Copy Markdown
Member

I need to put this back into my review queue!

@robsdedude

robsdedude commented Dec 14, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

FWIW, there already have been

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.

@AlexWaygood AlexWaygood changed the title GH-86296: Fix for wait_for() swallowing cancellation, and add tests Dec 14, 2022
@bedevere-bot bedevere-bot mentioned this pull request Dec 14, 2022
Closed
@netlify

netlify Bot commented Dec 14, 2022 •
edited
Loading

Copy link
Copy Markdown

✅ 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

kumaraditya303 commented Mar 14, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

robsdedude commented Mar 14, 2023

Copy link
Copy Markdown
Contributor

@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

gvanrossum commented Mar 14, 2023

Copy link
Copy Markdown
Member

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

kumaraditya303 commented Mar 14, 2023

Copy link
Copy Markdown
Contributor

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

gvanrossum commented Mar 14, 2023

Copy link
Copy Markdown
Member

So let's just close this.

@gvanrossum gvanrossum closed this Mar 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@1st1 1st1 Awaiting requested review from 1st1 1st1 is a code owner

@asvetlov asvetlov Awaiting requested review from asvetlov asvetlov is a code owner

@gvanrossum gvanrossum Awaiting requested review from gvanrossum

@kumaraditya303 kumaraditya303 Awaiting requested review from kumaraditya303 kumaraditya303 is a code owner

Assignees

No one assigned

Labels

awaiting review tests Tests in the Lib/test dir topic-asyncio

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@twisteroidambassador @gvanrossum @robsdedude @kumaraditya303 @bedevere-bot @AlexWaygood