Skip to content

GH-95704: Don't suppress errors from tasks when TG is cancelled #95761

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
gvanrossum merged 6 commits into from
Aug 17, 2022
Merged

GH-95704: Don't suppress errors from tasks when TG is cancelled #95761

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
44f6fa0
GH-95704: Don't suppress errors from tasks when TG is cancelled
gvanrossum Aug 7, 2022
afeab71
📜🤖 Added by blurb_it.
blurb-it[bot] Aug 8, 2022
4698b4b
Improve comment explaining when/why we propagate cancellations
gvanrossum Aug 8, 2022
6763dda
Add markup to NEWS file
gvanrossum Aug 8, 2022
79a4972
Never ignore side errors
gvanrossum Aug 10, 2022
63a1956
Remove blank line from middle of blurb
gvanrossum Aug 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions Lib/asyncio/taskgroups.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,10 +115,11 @@ 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 and we're not required by uncancel() != 0.
# (The latter means we *must* propagate the cancellation.)
if (propagate_cancellation_error is not None and

@graingert graingert Aug 8, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this now suppress the TimeoutError when used with async with timeout():?

I'll try and make a repro

@gvanrossum gvanrossum Aug 8, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the condition is true in fewer cases than the original code (where it was just if propagate_cancellation_error is not None:) I don't see how it could suppress something that wouldn't be suppressed previously. But I'm not sure what you're thinking of, so please make a repro.

(self._parent_cancel_requested or not self._errors)):
raise propagate_cancellation_error

if et is not None and et is not exceptions.CancelledError:
Expand Down
48 changes: 26 additions & 22 deletions Lib/test/test_asyncio/test_taskgroups.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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):

Expand Down Expand Up @@ -315,33 +315,37 @@ 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():
async with taskgroups.TaskGroup() as g2:
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:
Expand All @@ -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):
Expand Down
2 changes: 2 additions & 0 deletions Misc/NEWS.d/next/Library/2022-08-08-01-42-11.gh-issue-95704.MOPFfX.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
When a task catches CancelledError and raises some other error,
Comment thread
gvanrossum marked this conversation as resolved.
Resolved
the other error should generally not silently be suppressed.