From 9fce599ef0e39fc67157a3ca3328c633803f4bc6 Mon Sep 17 00:00:00 2001 From: Thomas Grainger Date: Mon, 20 Jan 2025 12:48:48 +0000 Subject: [PATCH 1/7] gh-128588: gh-128550: remove eager tasks optimization that missed and introduced incorrect cancellations --- Lib/asyncio/taskgroups.py | 12 +++---- Lib/test/test_asyncio/test_taskgroups.py | 44 ++++++++++++++++++++++++ 2 files changed, 49 insertions(+), 7 deletions(-) diff --git a/Lib/asyncio/taskgroups.py b/Lib/asyncio/taskgroups.py index 8af199d6dcc41a..8fda6c8d55e16c 100644 --- a/Lib/asyncio/taskgroups.py +++ b/Lib/asyncio/taskgroups.py @@ -197,14 +197,12 @@ def create_task(self, coro, *, name=None, context=None): else: task = self._loop.create_task(coro, name=name, context=context) - # optimization: Immediately call the done callback if the task is + # Always schedule the done callback even if the task is # already done (e.g. if the coro was able to complete eagerly), - # and skip scheduling a done callback - if task.done(): - self._on_task_done(task) - else: - self._tasks.add(task) - task.add_done_callback(self._on_task_done) + # otherwise if the task completes with an exception then it will cancel + # the current task too early. gh-128550, gh-128588 + self._tasks.add(task) + task.add_done_callback(self._on_task_done) try: return task finally: diff --git a/Lib/test/test_asyncio/test_taskgroups.py b/Lib/test/test_asyncio/test_taskgroups.py index 870fa8dbbf2714..348dd60324284f 100644 --- a/Lib/test/test_asyncio/test_taskgroups.py +++ b/Lib/test/test_asyncio/test_taskgroups.py @@ -1041,6 +1041,50 @@ class MyKeyboardInterrupt(KeyboardInterrupt): self.assertListEqual(gc.get_referrers(exc), no_other_refs()) + async def test_cancels_task_if_created_during_creation(self): + # regression test for gh-128550 + ran = False + class MyError(Exception): + pass + + exc = None + try: + async with asyncio.TaskGroup() as tg: + async def third_task(): + raise MyError("third task failed") + + async def second_task(): + nonlocal ran + tg.create_task(third_task()) + with self.assertRaises(asyncio.CancelledError): + await asyncio.sleep(0) # eager tasks cancel here + await asyncio.sleep(0) # lazy tasks cancel here + ran = True + + tg.create_task(second_task()) + except* MyError as excs: + exc = excs.exceptions[0] + + self.assertTrue(ran) + self.assertIsInstance(exc, MyError) + + + async def test_cancellation_does_not_leak_out_of_tg(self): + class MyError(Exception): + pass + + async def throw_error(): + raise MyError + + try: + async with asyncio.TaskGroup() as tg: + tg.create_task(throw_error()) + except* MyError: + pass + + await asyncio.sleep(0) + + class TestTaskGroup(BaseTestTaskGroup, unittest.IsolatedAsyncioTestCase): loop_factory = asyncio.EventLoop From 73042cf0414e2f97fa0c225bac75fd75159af2df Mon Sep 17 00:00:00 2001 From: "blurb-it[bot]" <43283697+blurb-it[bot]@users.noreply.github.com> Date: Mon, 20 Jan 2025 13:12:47 +0000 Subject: [PATCH 2/7] =?UTF-8?q?=F0=9F=93=9C=F0=9F=A4=96=20Added=20by=20blu?= =?UTF-8?q?rb=5Fit.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../next/Library/2025-01-20-13-12-39.gh-issue-128550.AJ5TOL.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 Misc/NEWS.d/next/Library/2025-01-20-13-12-39.gh-issue-128550.AJ5TOL.rst diff --git a/Misc/NEWS.d/next/Library/2025-01-20-13-12-39.gh-issue-128550.AJ5TOL.rst b/Misc/NEWS.d/next/Library/2025-01-20-13-12-39.gh-issue-128550.AJ5TOL.rst new file mode 100644 index 00000000000000..c208f587dcde3f --- /dev/null +++ b/Misc/NEWS.d/next/Library/2025-01-20-13-12-39.gh-issue-128550.AJ5TOL.rst @@ -0,0 +1 @@ +remove a problematic optimization relating to eager tasks in :class:`asyncio.TaskGroup` that resulted in cancellations being missed From 867bacbc752a94fcd50f4f13cad4c33b9240f3d9 Mon Sep 17 00:00:00 2001 From: Thomas Grainger Date: Mon, 20 Jan 2025 14:06:45 +0000 Subject: [PATCH 3/7] Update Lib/test/test_asyncio/test_taskgroups.py --- Lib/test/test_asyncio/test_taskgroups.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Lib/test/test_asyncio/test_taskgroups.py b/Lib/test/test_asyncio/test_taskgroups.py index 348dd60324284f..5374d0ed13bafe 100644 --- a/Lib/test/test_asyncio/test_taskgroups.py +++ b/Lib/test/test_asyncio/test_taskgroups.py @@ -1081,6 +1081,8 @@ async def throw_error(): tg.create_task(throw_error()) except* MyError: pass + else: + self.fail("should have raised one MyError in group") await asyncio.sleep(0) From c460fa9e45217dbf2efec8295d4dc9065663f41d Mon Sep 17 00:00:00 2001 From: Thomas Grainger Date: Mon, 20 Jan 2025 16:27:49 +0000 Subject: [PATCH 4/7] Update Lib/test/test_asyncio/test_taskgroups.py --- Lib/test/test_asyncio/test_taskgroups.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/Lib/test/test_asyncio/test_taskgroups.py b/Lib/test/test_asyncio/test_taskgroups.py index 5374d0ed13bafe..bdd86ecba289b0 100644 --- a/Lib/test/test_asyncio/test_taskgroups.py +++ b/Lib/test/test_asyncio/test_taskgroups.py @@ -1084,6 +1084,10 @@ async def throw_error(): else: self.fail("should have raised one MyError in group") + # if this test fails this current task will be cancelled + # outside the TG and inside unittest internals + # we add somewhere to cancel here so the test failure message + # is more understandable await asyncio.sleep(0) From 187abc71a15d66c44ac54650cfd8c94f35f63aaa Mon Sep 17 00:00:00 2001 From: Thomas Grainger Date: Mon, 20 Jan 2025 16:28:05 +0000 Subject: [PATCH 5/7] Update Misc/NEWS.d/next/Library/2025-01-20-13-12-39.gh-issue-128550.AJ5TOL.rst Co-authored-by: Kumar Aditya --- .../next/Library/2025-01-20-13-12-39.gh-issue-128550.AJ5TOL.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Misc/NEWS.d/next/Library/2025-01-20-13-12-39.gh-issue-128550.AJ5TOL.rst b/Misc/NEWS.d/next/Library/2025-01-20-13-12-39.gh-issue-128550.AJ5TOL.rst index c208f587dcde3f..0cc93e9adda254 100644 --- a/Misc/NEWS.d/next/Library/2025-01-20-13-12-39.gh-issue-128550.AJ5TOL.rst +++ b/Misc/NEWS.d/next/Library/2025-01-20-13-12-39.gh-issue-128550.AJ5TOL.rst @@ -1 +1 @@ -remove a problematic optimization relating to eager tasks in :class:`asyncio.TaskGroup` that resulted in cancellations being missed +Fixed an incorrect optimization relating to eager tasks in :class:`asyncio.TaskGroup` that resulted in cancellations being missed. From 874f7580c6556aace6da14b1b54729332ae42b91 Mon Sep 17 00:00:00 2001 From: Kumar Aditya Date: Mon, 20 Jan 2025 22:01:34 +0530 Subject: [PATCH 6/7] Update Lib/test/test_asyncio/test_taskgroups.py --- Lib/test/test_asyncio/test_taskgroups.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Lib/test/test_asyncio/test_taskgroups.py b/Lib/test/test_asyncio/test_taskgroups.py index bdd86ecba289b0..e226c35b956d2d 100644 --- a/Lib/test/test_asyncio/test_taskgroups.py +++ b/Lib/test/test_asyncio/test_taskgroups.py @@ -1085,9 +1085,9 @@ async def throw_error(): self.fail("should have raised one MyError in group") # if this test fails this current task will be cancelled - # outside the TG and inside unittest internals - # we add somewhere to cancel here so the test failure message - # is more understandable + # outside the task group and inside unittest internals + # we yield to the event loop with sleep(0) so that + # cancellation happens here and error is more understandable await asyncio.sleep(0) From 74ef5cccf06390a5b072e837f9088c2d203da1c9 Mon Sep 17 00:00:00 2001 From: Kumar Aditya Date: Mon, 20 Jan 2025 22:02:01 +0530 Subject: [PATCH 7/7] Update Misc/NEWS.d/next/Library/2025-01-20-13-12-39.gh-issue-128550.AJ5TOL.rst Co-authored-by: Thomas Grainger --- .../next/Library/2025-01-20-13-12-39.gh-issue-128550.AJ5TOL.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Misc/NEWS.d/next/Library/2025-01-20-13-12-39.gh-issue-128550.AJ5TOL.rst b/Misc/NEWS.d/next/Library/2025-01-20-13-12-39.gh-issue-128550.AJ5TOL.rst index 0cc93e9adda254..f59feac795ea18 100644 --- a/Misc/NEWS.d/next/Library/2025-01-20-13-12-39.gh-issue-128550.AJ5TOL.rst +++ b/Misc/NEWS.d/next/Library/2025-01-20-13-12-39.gh-issue-128550.AJ5TOL.rst @@ -1 +1 @@ -Fixed an incorrect optimization relating to eager tasks in :class:`asyncio.TaskGroup` that resulted in cancellations being missed. +Removed an incorrect optimization relating to eager tasks in :class:`asyncio.TaskGroup` that resulted in cancellations being missed.