GeezerVison is showing github.com — asyncio.TaskGroup corrupts uncancel() stack when the parent task replaces the cancelled error · Issue #95289 · python/cpython
asyncio.TaskGroup corrupts uncancel() stack when the parent task replaces the cancelled error #95289Description
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()) Activitygraingert commented on Jul 26, 2022 ContributorAuthor More actions 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 commented on Jul 26, 2022 ContributorAuthor More actions cc @ambv who helped me find this graingert commented on Jul 27, 2022 ContributorAuthor More actions YvesDup commented on Jul 27, 2022 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 commented on Jul 27, 2022 ContributorAuthor More actions
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 commented on Jul 29, 2022 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 commented on Jul 29, 2022 @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 commented on Jul 29, 2022 Have you tried this on the edgedb's task group 1 implementation from which the current one is derived from?
gvanrossum commented on Jul 30, 2022 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 commented on Jul 31, 2022 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 commented on Jul 31, 2022 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 commented on Aug 1, 2022 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?) added a commit that references this issue on Aug 4, 2022GH-95289: Always call uncancel() when parent cancellation is requested ( added 2 commits that reference this issue on Aug 4, 2022pythonGH-95289: Always call uncancel() when parent cancellation is re… GH-95289: Always call uncancel() when parent cancellation is requested ( kumaraditya303 commented on Aug 4, 2022
MetadataMetadataStatusShow more project fields
DevelopmentNo branches or pull requestsIssue actions
|