asyncio.TaskGroup corrupts uncancel() stack when the parent task replaces the cancelled error

[graingert]

Bug report

import asyncio

class MyException(Exception):
    pass

async def async_fn():
    await asyncio.sleep(0)
    raise MyException

async def main():
    task = asyncio.current_task()

    try:
        async with asyncio.TaskGroup() as tg:
            tg.create_task(async_fn())
            try:
                await asyncio.sleep(1)
            except asyncio.CancelledError:
                pass
    except* MyException:
        print("done!")

    print(f"{task.cancelling()=} should be 0")

asyncio.run(main())

[graingert]

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())

[graingert]

cc @ambv who helped me find this

[graingert]

Tagging @pablogsal as potential release-blocker

[YvesDup]

Hello, sorry for my question but I have never seen a * after an except statement. I couldn't find an explanation on the web. What does it mean, please?

In your first example, if you replace pass with raise, you have the expected result.
May be this will help you ?

[graingert]

Hello, sorry for my question but I have never seen a * after an except statement. 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"

[YvesDup]

Hi @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 ?

[ambv]

@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 with we want print(f"{task.cancelling()=} should be 0").

[kumaraditya303]

Have you tried this on the edgedb's task group ¹ implementation from which the current one is derived from?

Footnotes

1. https://github.com/edgedb/edgedb/blob/master/edb/common/taskgroup.py ↩

[1st1]

@graingert I see that you're suppressing CancelledError in your OP. Why? Why is this a release blocker?

[1st1]

@ambv @pablogsal can this wait until Monday? I can debug this just not right now.

[pablogsal]

@ambv @pablogsal can this wait until Monday? I can debug this just not right now.

Yep, the release is delayed until next Friday so there is plenty of time

[YvesDup]

@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 with we want print(f"{task.cancelling()=} should be 0").

@ambv I trust you but this part of the documentation is difficult to understand. Il would be nice to have a more explict description or/and an example.

[gvanrossum]

I can report that with edb's TaskGroup this also fails (you have to change the except to except Exception).

[gvanrossum]

The key issue seems to be that we're entering __aexit__() with self._parent_cancel_requested set but an exception that's not CancelledError. So we never take the path that calls uncancel(). I'm not at all sure how to fix this. Always calling uncancel() if that flag is set would seem too easy...

[gvanrossum]

Here's a remarkably simple patch that seems to fix the issue. I'm not sure of it yet.

diff --git a/Lib/asyncio/taskgroups.py b/Lib/asyncio/taskgroups.py
index 3ca65062ef..7b8886a205 100644
--- a/Lib/asyncio/taskgroups.py
+++ b/Lib/asyncio/taskgroups.py
@@ -62,12 +62,10 @@ async def __aexit__(self, et, exc, tb):
             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 not self._aborting:
                 # Our parent task is being cancelled:

[gvanrossum]

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?

[gvanrossum]

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.

[gvanrossum]

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.

[gvanrossum]

Oh, I think I understand why that doesn't matter. In this case we end up returning None from __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?)

[kumaraditya303]

Fixed by #95602