From 44f6fa0b48467bcfb9ead7e99c2a22ff3b61fecf Mon Sep 17 00:00:00 2001 From: Guido van Rossum Date: Sun, 7 Aug 2022 08:35:27 -0700 Subject: [PATCH 1/6] GH-95704: Don't suppress errors from tasks when TG is cancelled When a task catches CancelledError and raises some other error, the other error should not silently be suppressed. Exception: when uncancel() != 0, always propagate CancelledError. NOTE: This represents a change in behavior (hence the need to change three tests). But it is only an edge case. We need to discuss with the RM whether to backport to 3.11RC2, or 3.11.1, or not at all. --- Lib/asyncio/taskgroups.py | 3 +- Lib/test/test_asyncio/test_taskgroups.py | 48 +++++++++++++----------- 2 files changed, 28 insertions(+), 23 deletions(-) diff --git a/Lib/asyncio/taskgroups.py b/Lib/asyncio/taskgroups.py index 097b4864f7ab98..145b38dd5c26de 100644 --- a/Lib/asyncio/taskgroups.py +++ b/Lib/asyncio/taskgroups.py @@ -115,7 +115,8 @@ async def __aexit__(self, et, exc, tb): if self._base_error is not None: raise self._base_error - if propagate_cancellation_error is not None: + if (propagate_cancellation_error is not None and + (self._parent_cancel_requested or not self._errors)): # The wrapping task was cancelled; since we're done with # closing all child tasks, just propagate the cancellation # request now. diff --git a/Lib/test/test_asyncio/test_taskgroups.py b/Lib/test/test_asyncio/test_taskgroups.py index 99498e7b36f0f9..8428ae444eb98e 100644 --- a/Lib/test/test_asyncio/test_taskgroups.py +++ b/Lib/test/test_asyncio/test_taskgroups.py @@ -229,29 +229,29 @@ async def runner(): self.assertEqual(NUM, 15) - async def test_cancellation_in_body(self): + async def test_taskgroup_08(self): async def foo(): - await asyncio.sleep(0.1) - 1 / 0 + try: + await asyncio.sleep(10) + finally: + 1 / 0 async def runner(): async with taskgroups.TaskGroup() as g: for _ in range(5): g.create_task(foo()) - try: - await asyncio.sleep(10) - except asyncio.CancelledError: - raise + await asyncio.sleep(10) r = asyncio.create_task(runner()) await asyncio.sleep(0.1) self.assertFalse(r.done()) r.cancel() - with self.assertRaises(asyncio.CancelledError) as cm: + with self.assertRaises(ExceptionGroup) as cm: await r + self.assertEqual(get_error_types(cm.exception), {ZeroDivisionError}) async def test_taskgroup_09(self): @@ -315,8 +315,10 @@ async def runner(): async def test_taskgroup_11(self): async def foo(): - await asyncio.sleep(0.1) - 1 / 0 + try: + await asyncio.sleep(10) + finally: + 1 / 0 async def runner(): async with taskgroups.TaskGroup(): @@ -324,24 +326,26 @@ async def runner(): for _ in range(5): g2.create_task(foo()) - try: - await asyncio.sleep(10) - except asyncio.CancelledError: - raise + await asyncio.sleep(10) r = asyncio.create_task(runner()) await asyncio.sleep(0.1) self.assertFalse(r.done()) r.cancel() - with self.assertRaises(asyncio.CancelledError): + with self.assertRaises(ExceptionGroup) as cm: await r + self.assertEqual(get_error_types(cm.exception), {ExceptionGroup}) + self.assertEqual(get_error_types(cm.exception.exceptions[0]), {ZeroDivisionError}) + async def test_taskgroup_12(self): async def foo(): - await asyncio.sleep(0.1) - 1 / 0 + try: + await asyncio.sleep(10) + finally: + 1 / 0 async def runner(): async with taskgroups.TaskGroup() as g1: @@ -351,19 +355,19 @@ async def runner(): for _ in range(5): g2.create_task(foo()) - try: - await asyncio.sleep(10) - except asyncio.CancelledError: - raise + await asyncio.sleep(10) r = asyncio.create_task(runner()) await asyncio.sleep(0.1) self.assertFalse(r.done()) r.cancel() - with self.assertRaises(asyncio.CancelledError): + with self.assertRaises(ExceptionGroup) as cm: await r + self.assertEqual(get_error_types(cm.exception), {ExceptionGroup}) + self.assertEqual(get_error_types(cm.exception.exceptions[0]), {ZeroDivisionError}) + async def test_taskgroup_13(self): async def crash_after(t): From afeab717de05bd3c7ca165cc26b1078f26506f0a Mon Sep 17 00:00:00 2001 From: "blurb-it[bot]" <43283697+blurb-it[bot]@users.noreply.github.com> Date: Mon, 8 Aug 2022 01:42:15 +0000 Subject: [PATCH 2/6] =?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/2022-08-08-01-42-11.gh-issue-95704.MOPFfX.rst | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 Misc/NEWS.d/next/Library/2022-08-08-01-42-11.gh-issue-95704.MOPFfX.rst diff --git a/Misc/NEWS.d/next/Library/2022-08-08-01-42-11.gh-issue-95704.MOPFfX.rst b/Misc/NEWS.d/next/Library/2022-08-08-01-42-11.gh-issue-95704.MOPFfX.rst new file mode 100644 index 00000000000000..42bfc834ce5cbd --- /dev/null +++ b/Misc/NEWS.d/next/Library/2022-08-08-01-42-11.gh-issue-95704.MOPFfX.rst @@ -0,0 +1,2 @@ +When a task catches CancelledError and raises some other error, +the other error should generally not silently be suppressed. From 4698b4b59786d2e1ed141204f41c48246b5689ae Mon Sep 17 00:00:00 2001 From: Guido van Rossum Date: Sun, 7 Aug 2022 18:56:36 -0700 Subject: [PATCH 3/6] Improve comment explaining when/why we propagate cancellations --- Lib/asyncio/taskgroups.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Lib/asyncio/taskgroups.py b/Lib/asyncio/taskgroups.py index 145b38dd5c26de..99925d03b9392c 100644 --- a/Lib/asyncio/taskgroups.py +++ b/Lib/asyncio/taskgroups.py @@ -115,11 +115,11 @@ async def __aexit__(self, et, exc, tb): if self._base_error is not None: raise self._base_error + # Propagate CancelledError if there is one, except if there + # are other errors and we're not required by uncancel() != 0. + # (The latter means we *must* propagate the cancellation.) if (propagate_cancellation_error is not None and (self._parent_cancel_requested or not self._errors)): - # The wrapping task was cancelled; since we're done with - # closing all child tasks, just propagate the cancellation - # request now. raise propagate_cancellation_error if et is not None and et is not exceptions.CancelledError: From 6763dda34e1621c968b35490e57bbd23a2eb6393 Mon Sep 17 00:00:00 2001 From: Guido van Rossum Date: Mon, 8 Aug 2022 08:03:46 -0700 Subject: [PATCH 4/6] Add markup to NEWS file Co-authored-by: Thomas Grainger --- .../Library/2022-08-08-01-42-11.gh-issue-95704.MOPFfX.rst | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/Misc/NEWS.d/next/Library/2022-08-08-01-42-11.gh-issue-95704.MOPFfX.rst b/Misc/NEWS.d/next/Library/2022-08-08-01-42-11.gh-issue-95704.MOPFfX.rst index 42bfc834ce5cbd..6d58310e4b26ee 100644 --- a/Misc/NEWS.d/next/Library/2022-08-08-01-42-11.gh-issue-95704.MOPFfX.rst +++ b/Misc/NEWS.d/next/Library/2022-08-08-01-42-11.gh-issue-95704.MOPFfX.rst @@ -1,2 +1,3 @@ -When a task catches CancelledError and raises some other error, -the other error should generally not silently be suppressed. +When a task catches :exc:`asyncio.CancelledError` and raises some other error, + +the other error should generally not silently be suppressed. From 79a49722d289089a56c9dfec5c203f655c3012b4 Mon Sep 17 00:00:00 2001 From: Guido van Rossum Date: Wed, 10 Aug 2022 12:54:30 -0700 Subject: [PATCH 5/6] Never ignore side errors Any scenario where a task crashes in cleanup upon cancellation will now result in an ExceptionGroup wrapping the crash(es) instead of propagating CancelledError and ignoring the side errors. --- Lib/asyncio/taskgroups.py | 6 ++---- Lib/test/test_asyncio/test_taskgroups.py | 6 ++++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/Lib/asyncio/taskgroups.py b/Lib/asyncio/taskgroups.py index 99925d03b9392c..c4d6b30e4d0643 100644 --- a/Lib/asyncio/taskgroups.py +++ b/Lib/asyncio/taskgroups.py @@ -116,10 +116,8 @@ async def __aexit__(self, et, exc, tb): raise self._base_error # Propagate CancelledError if there is one, except if there - # are other errors and we're not required by uncancel() != 0. - # (The latter means we *must* propagate the cancellation.) - if (propagate_cancellation_error is not None and - (self._parent_cancel_requested or not self._errors)): + # are other errors -- those have priority. + if propagate_cancellation_error and not self._errors: raise propagate_cancellation_error if et is not None and et is not exceptions.CancelledError: diff --git a/Lib/test/test_asyncio/test_taskgroups.py b/Lib/test/test_asyncio/test_taskgroups.py index 8428ae444eb98e..12aefbaa5cf27b 100644 --- a/Lib/test/test_asyncio/test_taskgroups.py +++ b/Lib/test/test_asyncio/test_taskgroups.py @@ -427,8 +427,9 @@ async def runner(): self.assertFalse(r.done()) r.cancel() - with self.assertRaises(asyncio.CancelledError): + with self.assertRaises(ExceptionGroup) as cm: await r + self.assertEqual(get_error_types(cm.exception), {ZeroDivisionError}) async def test_taskgroup_16(self): @@ -454,8 +455,9 @@ async def runner(): self.assertFalse(r.done()) r.cancel() - with self.assertRaises(asyncio.CancelledError): + with self.assertRaises(ExceptionGroup) as cm: await r + self.assertEqual(get_error_types(cm.exception), {ZeroDivisionError}) async def test_taskgroup_17(self): NUM = 0 From 63a19561147cc468a024fa67b22cdb5c61421d82 Mon Sep 17 00:00:00 2001 From: Guido van Rossum Date: Tue, 16 Aug 2022 16:49:17 -0700 Subject: [PATCH 6/6] Remove blank line from middle of blurb --- .../next/Library/2022-08-08-01-42-11.gh-issue-95704.MOPFfX.rst | 1 - 1 file changed, 1 deletion(-) diff --git a/Misc/NEWS.d/next/Library/2022-08-08-01-42-11.gh-issue-95704.MOPFfX.rst b/Misc/NEWS.d/next/Library/2022-08-08-01-42-11.gh-issue-95704.MOPFfX.rst index 6d58310e4b26ee..31f9fc6547d90f 100644 --- a/Misc/NEWS.d/next/Library/2022-08-08-01-42-11.gh-issue-95704.MOPFfX.rst +++ b/Misc/NEWS.d/next/Library/2022-08-08-01-42-11.gh-issue-95704.MOPFfX.rst @@ -1,3 +1,2 @@ When a task catches :exc:`asyncio.CancelledError` and raises some other error, - the other error should generally not silently be suppressed.