Repository navigation
TaskGroup suppresses grouped exceptions due to parent task cancellation #95704
Description
Activity
I walked through this one with a debugger and you're right that we can do better. It seems the essence of the reproducer is that we enter
TaskGroup__aexit__with aCancellationErrorbut without having cancelled any tasks yet, and then one or more tasks catch theCancellationErrorand raise something else (in your case,AttributeErrorbecausecontextlib.closing()callssock.close()during cleanup, which doesn't exist). This code seems to repro the same issue but without a fake socket class or context managers, and with only one child:import asyncio async def child(): try: await asyncio.sleep(2) except asyncio.CancelledError: print("** Cancelled **") raise RuntimeError async def main(): async with asyncio.TaskGroup() as tg: tg.create_task(child()) await asyncio.sleep(0.5) raise asyncio.CancelledError asyncio.run(main())
It prints
** Cancelled **and then raisesCancelledError, whereas I agree that it would be more useful to report theRuntimeErrorAlas, the obvious fix (don't honor
propagate_cancellation_errorifself._errorsis non-empty) breaks two of the asyncio tests.Looks like this is the correct fix:
--- 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.
@grangert Do you agree? If so, do you want to make a PR with the new test added?
Spoke too soon, now
test_cancellation_in_bodyfails. :-(And so does
test_taskgroup_11. It's a game of whack-a-mole.Oh wait, sometimes I get no failures, sometimes I get
test_taskgroup_12failing. FWIW I am testing on Windows.Interestingly
I don't get any failures on MacUPDATE: it crashes much less frequently on Mac.What do you get on Linux?So
test_taskgroup_11has a race condition. Each of the 5 tasks created sleeps 0.1 second and then crashes, raisingZeroDivisionError. The main task sleeps 0.1 second and then cancels the runner task, which cancels the crashing tasks. The test is written to always expect aCancelledError, but if any of the 5 crashing tasks finishes its sleep before being cancelled, it will raiseZeroDivisionErrorand the change toTaskGroup.__aexit__makes it so that in that cases it raises anExceptionGroupwrapping theZeroDivisionError(s).IIUC
test_taskgroup_12has the same race, the difference withtest_taskgroup_11is that it uses two nestedTaskGroups.It looks like
test_cancellation_in_bodyhas the same race condition again. (This looks like it used to be calledtest_taskgroup_08in EdgeDb; @Tinche renamed it when he added a cancel message to it in #31559 -- which was then removed again by @asvetlov when he deprecated cancel messages. :-)Now I'd love to ask @1st1 what his intention was with these tests -- did he want them to be "racy"? (That would have been annoying, because racy tests just destabilize the test suite.) Assuming not, is the scenario the tests attempts to test the one where the crashers raise
ZeroDivisionError, or the one where they are cancelled in their sleep?Discuss.
Going through the thread. Going by the 'errors should never pass silently' I also agree we should make it so the example from #95704 (comment) raises an EG with
RuntimeError.Assuming not, is the scenario the tests attempts to test the one where the crashers raise ZeroDivisionError, or the one where they are cancelled in their sleep?
I copy/pasted those tests into quattro, and at that time for
test_taskgroup_08I added the following docstring: """Cancelling a TG does not propagate children exceptions.""" (but I wrote it as my interpretation of the test). So I'm guessing the children were supposed to raise, and those exceptions were supposed to get swallowed.Oh, and the quattro test isn't flaky, at least I couldn't make it fail. But it is racy, sometimes the children get cancelled and sometimes they don't, but the test passes in both cases, so it's still a bad test.
Thanks, @Tinche. What would quattro do with the example from #95704 (comment) (assuming that's a meaningful question)? Does it raise
CancelledErroror (an EG wrapping a)RuntimeError? If it raisesCancelledError, that explains why the test isn't flaky for quattro -- it's not flaky in asyncio on main either, but it would be if we were to commit my fix totaskgroups.pywithout fixing those three racy tests.Your docstring might well express the intent of the test, but @graingert's complaint here is that cancelling a TG should propagate the children's exceptions.
Come to think of it, maybe this intent explains the raciness of the test: the idea might have been to cancel the test before any of the children have crashed, but to have at least one child crash before the cancellation reaches
TaskGroup.__aexit__. (Hence creating 5 children -- to increase the chance of at least one of them actually crashing.) @graingert's demo (or my reduction of it) would seem to be a much more reliable way to elicit this effect: the child task crashes during cleanup, after it has caught theCancelledError.So I'm guessing the children were supposed to raise, and those exceptions were supposed to get swallowed.
But they weren't supposed to raise too soon -- note the line (after the main task has slept 0.1 seconds, and just before it cancels the runner task)
self.assertFalse(r.done())
which indicates that we expect the runner to still be running -- IOW the TG hasn't exited yet, which looks like a proxy for the assertion that the TG hasn't processed any crashing child tasks yet, and that in turn is attempting to assert that no child task has crashed yet at this point.
So shortening the sleep in the crasher task would defeat the purpose of the test, and make it fail -- the TG would self-cancel when the first child crashes before the main task can cancel the runner task. But lengthening the sleep in the crasher would also defeat the purpose of the test, while making it pass -- the crasher tasks would never crash.
In conclusion I think the three affected tests might have to be modified to use the following crasher:
async def foo(): try: await asyncio.sleep(1) finally: 1 / 0
and they would have to be changed to expect an EG wrapping
ZeroDivisionErrorinstead ofCancellationError(not wrapped by an EG).What would quattro do with the example from #95704 (comment)
It raises
CancelledError. But you have to remember the quattro TG is the EdgeDB TG with minimal changes (type hints and tweaks).Your docstring might well express the intent of the test, but @graingert's complaint here is that cancelling a TG should propagate the children's exceptions.
Yeah, I think it makes sense to change it in asyncio, and quattro will follow whatever asyncio does here.
Come to think of it, maybe this intent explains the raciness of the test
I wager you're right. I personally wouldn't write the test this way though, I would write several tests for each scenario, so I vote for rewriting these tests.
In conclusion I think the three affected tests might have to be modified
Can we modify the tests to avoid sleeps with these hardcoded timeouts?
What about rewriting
test_cancellation_in_bodylike this:async def test_cancellation_in_body_child_propagation(self): """ If the task running the TaskGroup is blocked on something and then cancelled, the TG children are cancelled too. If the children raise an exception different than `CancelledError` while being stopped, those exceptions are raised in an ExceptionGroup. """ LONG_SLEEP = 10 # The awaiter is expected to be cancelled waiting on this. exc = RuntimeError() async def foo(): try: await asyncio.sleep(LONG_SLEEP) except CancelledError: # We raise, and expect it to be propagated. raise exc # Something other than a CancelledError. async def runner(): async with taskgroups.TaskGroup() as g: g.create_task(foo()) await asyncio.sleep(LONG_SLEEP) r = asyncio.create_task(runner()) await asyncio.sleep(0.1) self.assertFalse(r.done()) r.cancel() with self.assertRaises(ExceptionGroup) as cm: await r self.assertEqual(cm.exception == ExceptionGroup([exc]))
(I didn't actually run this because I don't have your changes, but you get the idea.)
Just about, although that's not the way to test for an EG.
TaskGroup suppresses grouped exceptions due to parent task cancellation
The suggestion was to always check for Cancellation in every finally block in async context managers, however sometimes there's no finally block visible and the problem applies to synchronous context managers too
In this demo I'd expect to be able to print: "handled attribute error"