Repository navigation
asyncio.TaskGroup corrupts uncancel() stack when the parent task replaces the cancelled error #95289
Description
Activity
this is also quite easy to trigger in typical task group code:
import asyncio import contextlib class ConnectionClosedError(Exception): pass async def poll_database(database): raise ConnectionClosedError async def close_connection(): raise ConnectionClosedError @contextlib.asynccontextmanager async def database(): try: yield finally: await close_connection() async def main(): task = asyncio.current_task() try: async with asyncio.TaskGroup() as tg: async with database() as db: tg.create_task(poll_database(db)) await asyncio.sleep(1) except* ConnectionClosedError: print("done!") print(f"{task.cancelling()=} should be 0") asyncio.run(main())
Tagging @pablogsal as potential release-blocker
Hello, sorry for my question but I have never seen a
*after anexceptstatement. I couldn't find an explanation on the web. What does it mean, please?In your first example, if you replacepasswithraise, you have the expected result.
May be this will help you ?Hello, sorry for my question but I have never seen a
*after anexceptstatement. I couldn't find an explanation on the web. What does it mean, please?Hi there - it's the new 3.11 ExceptionGroup feature https://peps.python.org/pep-0654/ it's required to handle an issue where multiple errors can happen concurrently. Eg a connection error canceles a group of concurrent operations, and the cancellation error leads to some teardown that also fails. In 3.10 library authors had to either choose an error to raise or wrap them in a list and raise a generic "MultiError"
Reacted by DupratHi @graingert, thank you for your response about
except *feature.About the issue, I read the documentation about 'Task groups' and IMHO there is a part about the two cases, in the second paragraph below the exemple:
... At this point, if the body of the async with statement is still active (
__aexit__()hasn’t been called yet), the task directly containing the async with statement is also cancelled.Do you think that your both examples match the situation described ?
@YvesDup, you stopped your quote just before the sentence that describes why what Thomas reported is a bug:
The resulting asyncio.CancelledError will interrupt an await, but it will not bubble out of the containing async with statement.
In other words, after leaving
async withwe wantprint(f"{task.cancelling()=} should be 0").Have you tried this on the edgedb's task group 1 implementation from which the current one is derived from?
Footnotes
7 remaining items
Alas, that doesn't fix the original bug report. But here's a variant that does:
diff --git a/Lib/asyncio/taskgroups.py b/Lib/asyncio/taskgroups.py index 3ca65062ef..0dde87f1ca 100644 --- a/Lib/asyncio/taskgroups.py +++ b/Lib/asyncio/taskgroups.py @@ -61,14 +61,12 @@ async def __aexit__(self, et, exc, tb): self._base_error is None): self._base_error = exc - if et is not None: - if et is exceptions.CancelledError: - if self._parent_cancel_requested and not self._parent_task.uncancel(): - # Do nothing, i.e. swallow the error. - pass - else: - propagate_cancellation_error = exc + if (self._parent_cancel_requested and + self._parent_task.uncancel() != 0 and + et is exceptions.CancelledError): + propagate_cancellation_error = exc + if et is not None: if not self._aborting: # Our parent task is being cancelled: #
I have other things to do, maybe @graingert and/or @kumaraditya303 can check my fix and turn it into a PR with the two examples as new tests?
Hm, there’s one case where the effect is different from the original: if a CancellationError occurs while _parent_cancel_request is not set. Then the original sets propagate_cancellation_error = exc but the new code does nothing. Does that matter? No tests fail.
That would affect the case where the parent task is cancelled externally — it should propagate the cancellation but with my code it will exit cleanly. This can be fixed and requires a separate test.
Oh, I think I understand why that doesn't matter. In this case we end up returning
Nonefrom__aexit__()and that causes the interpreter to re-raise the original exception that was being handled. (In fact we may use this fact to simplify the code even further?)- added a commit that references this issue
on Aug 3, 2022 - added a commit that references this issue
on Aug 4, 2022 Fixed by #95602
Bug report