From 34b840f4feaf5cb40367545a1413b315bb2c0ce6 Mon Sep 17 00:00:00 2001 From: Guido van Rossum Date: Tue, 16 Aug 2022 18:23:06 -0700 Subject: [PATCH] GH-95704: Don't suppress errors from tasks when TG is cancelled (GH-95761) When a task catches CancelledError and raises some other error, the other error should not silently be suppressed. 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. NOTE: This represents a change in behavior (hence the need to change several tests). But it is only an edge case. Co-authored-by: Thomas Grainger (cherry picked from commit f51f54f39d384da63be622bcdc9cf4cfb43bad3d) Co-authored-by: Guido van Rossum --- Lib/asyncio/taskgroups.py | 7 ++- Lib/test/test_asyncio/test_taskgroups.py | 54 ++++++++++--------- ...2-08-08-01-42-11.gh-issue-95704.MOPFfX.rst | 2 + 3 files changed, 35 insertions(+), 28 deletions(-) create mode 100644 Misc/NEWS.d/next/Library/2022-08-08-01-42-11.gh-issue-95704.MOPFfX.rst diff --git a/Lib/asyncio/taskgroups.py b/Lib/asyncio/taskgroups.py index 9be4838e3c7ad0..5d5e2a8a85dd48 100644 --- a/Lib/asyncio/taskgroups.py +++ b/Lib/asyncio/taskgroups.py @@ -116,10 +116,9 @@ 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: - # The wrapping task was cancelled; since we're done with - # closing all child tasks, just propagate the cancellation - # request now. + # Propagate CancelledError if there is one, except if there + # 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 74bae06af8e729..6a0231f2859a62 100644 --- a/Lib/test/test_asyncio/test_taskgroups.py +++ b/Lib/test/test_asyncio/test_taskgroups.py @@ -230,29 +230,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): @@ -316,8 +316,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(): @@ -325,24 +327,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: @@ -352,19 +356,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): @@ -424,8 +428,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): @@ -451,8 +456,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 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..31f9fc6547d90f --- /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 :exc:`asyncio.CancelledError` and raises some other error, +the other error should generally not silently be suppressed.