FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

GH-86296: Fix for asyncio.wait_for() swallowing cancellation, and add tests by twisteroidambassador · Pull Request #98607 · python/cpython · GitHub

/ cpython Public

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
ConversationCommits (7)ChecksFiles changed
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 commented Oct 24, 2022 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Contributor

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

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

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 mentioned this pull request Oct 25, 2022
Merged
Remove tests for simultaneous *timeout* and *inner becoming done*.
c834122 
The existing method of setting identical timeout / sleep lengths is not reliable.
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 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 commented Nov 30, 2022

Copy link
Copy Markdown
Member

I need to put this back into my review queue!

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 changed the title GH-86296: Fix for wait_for() swallowing cancellation, and add tests GH-86296: Fix for asyncio.wait_for() swallowing cancellation, and add tests Dec 14, 2022
bedevere-bot mentioned this pull request Dec 14, 2022
Closed

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 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 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 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 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 commented Mar 14, 2023

Copy link
Copy Markdown
Member

So let's just close this.

gvanrossum closed this Mar 14, 2023
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1st1 Awaiting requested review from 1st1 1st1 is a code owner
asvetlov Awaiting requested review from asvetlov asvetlov is a code owner
gvanrossum Awaiting requested review from gvanrossum
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

Back | FazBrowse Home | New Git URL