Repository navigation
Catching CancelledError requires use of task.uncancel() #102780
Description
Bug report
Today I came across the following problem:
import asyncio
ready = asyncio.Event()
async def task_func():
try:
ready.set()
await asyncio.sleep(1)
except asyncio.CancelledError:
# this is required, otherwise a TimeoutError is not created below,
# instead, the timeout-generated CancelledError is just raised upwards.
asyncio.current_task().uncancel()
finally:
# Perform some cleanup, with a timeout
try:
async with asyncio.timeout(0):
await asyncio.sleep(1)
except asyncio.TimeoutError:
pass
async def main():
task = asyncio.create_task(task_func())
await ready.wait()
await asyncio.sleep(0.01)
# the task is now sleeping, lets send it an exception
task.cancel()
# We expect the task to finish cleanly.
await task
asyncio.run(main())The documentation mentions that sometimes an application may want to suppress CancelledError. What it fails
to mention is that unless the cancel state is subsequently cancelled with task.uncancel(), the task remains in a
cancelled state, and context managers such as asyncio.timeout will not work as designed. However, task.uncancel()
is not supposed to be called by user code.
I think the documentation should mention this use case for task.uncancel(), particularly in the context of catching (and choosing to ignore) CancelledError.
This could also be considered a bug in asyncio.timeout. It prevents the use of the Timeout context manager within cleanup code, even if the intention is not to ignore a CancelledError.
It should also be noted that the library asyncio_timeout on which the asyncio.timeout implementation is based, does not have this problem. Timeouts continue to work as designed, even if the task has been previously cancelled.
Your environment
- CPython versions tested on: 3.11
Activity
gvanrossum commentedon Mar 17, 2023
This should be treated as a documentation issue. The mechanism is essential for task groups.
kristjanvalur commentedon Mar 17, 2023
Yes, I wasn't quite sure about that.
Still, it means that cleanup code cannot use timeouts, if cleaning up after a
cancel(). And the use of timeout context managers is widespread in asyncio libraries.gvanrossum commentedon Mar 17, 2023
If you can propose a fix to the timeout code that would be great, I wasn’t aware of the issue there.
kristjanvalur commentedon Mar 17, 2023
This is the code in question:
The test for the
self._task.uncancel()return value is what is causing the problem. This was presumably added for a good reason, but its unclear to me what it is. Maybe whoever implemented this can comment?I guess I could at least provide a PR for the docs, suggest that if the user chooses to suppress a
CancelledErrorhe must also calluncancel(). I don't know what implication that might or might not have for task groups, though.gvanrossum commentedon Mar 17, 2023
Unfortunately @asvetlov is busy. Since I reviewed it I could try to have a look but it's been a while since I understood that code. Comparison to how and why task groups use uncancel() might help.
The proposed doc update sounds good.
kristjanvalur commentedon Mar 18, 2023
I think I know what is going on. Because exceptions are delivered asynchronously, the intent here is to guard against a race condition: A timeout expires, a cancel is scheduled to be delivered. At the same time, a proper cancel is also issued. We want the proper cancel to propagate and not be turned into a timeout error.
I think this is fixable, at least in the majority of cases, by comparing the cancel counter before and after entering. I'll see if I can make that happen without making too much rubble.
On the other hand, I think this also indicates what I think is a shortcoming with the asyncio api: There is currently no mechanism to send a general exception to a task. There is only
cancel(), which sends one type of exception.In
stacklesslibwe had a similar timeout mechanism, but it arranged for a custom exception to be sent to the tasklet. This exception had the triggering context manager instance as a an instance variable so there was no need to perform logic to deduce if it should be turned into a TimeoutError or not.I have previously stopped short working with asyncio wanting to send a custom exception and not finding it possible. I would suggest that such an api be added. Even
cancel()would benefit from having at least a subclass instance being optionally provided.A complication in asyncio vs stackless is that in the latter, the exceptions could be raised synchronously. There was no race, where different exceptions might be sent to a tasklet, each overriding the previous. In asyncio, this could be accomplished by considering the
CancelledErroras special (which is already the case) and not having other exceptions override it.Implementing a timeout by simply delivering a custom
BaseExceptionand not messing with the cancel mechanism (which looks very complex at this point, I just had a look at the source) would be far simpler. I don't think co-opting CancelledError is very sound anymore.Anyway, just a few thoughts I wanted to get out of my head :)
kumaraditya303 commentedon Mar 18, 2023
Seems worth looking into where's the difference between the two. I'll take a look at this and PR in more detail in next week.
gvanrossum commentedon Mar 18, 2023
This is at least worth a discussion, but I'd rather not continue in this thread. Could you create a new thread with the same thoughts? Given how much we've been struggling with cancellation I agree it deserves more consideration.
gh-102780: Fix uncancel() call in asyncio timeouts (#102815)
pythongh-102780: Fix uncancel() call in asyncio timeouts (pythonGH-10…
gvanrossum commentedon Mar 22, 2023
Thanks for the fix, Kristjan. (And welcome back!) I'm backporting this to 3.11 as a bugfix.
gh-102780: Fix uncancel() call in asyncio timeouts (GH-102815)
kristjanvalur commentedon Mar 23, 2023
Thanks. I'm never far off, and when I notice curious and strange behaviour in Python, I try to help fix it. My first core fix in many years was in #95207
pythongh-102780: Fix uncancel() call in asyncio timeouts (python#102815)
pythongh-102780: Fix uncancel() call in asyncio timeouts (python#102815)
Switch all uses of async_timeout to asyncio.timeout with py3.11+
Pull in asyncio.timeout implementation from Python 3.11.3 (SYN-6087) (#…