Catching CancelledError requires use of task.uncancel()

[kristjanvalur]

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

Linked PRs

• gh-102780: Fix uncancel() call in asyncio timeouts #102815
• [3.11] gh-102780: Fix uncancel() call in asyncio timeouts (GH-102815) #102923

[gvanrossum]

This should be treated as a documentation issue. The mechanism is essential for task groups.

[kristjanvalur]

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]

If you can propose a fix to the timeout code that would be great, I wasn’t aware of the issue there.

[kristjanvalur]

This is the code in question:

async def __aexit__(
        self,
        exc_type: Optional[Type[BaseException]],
        exc_val: Optional[BaseException],
        exc_tb: Optional[TracebackType],
    ) -> Optional[bool]:
        assert self._state in (_State.ENTERED, _State.EXPIRING)

        if self._timeout_handler is not None:
            self._timeout_handler.cancel()
            self._timeout_handler = None

        if self._state is _State.EXPIRING:
            self._state = _State.EXPIRED

            if self._task.uncancel() == 0 and exc_type is exceptions.CancelledError:
                # Since there are no outstanding cancel requests, we're
                # handling this.
                raise TimeoutError
        elif self._state is _State.ENTERED:
            self._state = _State.EXITED

        return None

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 CancelledError he must also call uncancel(). I don't know what implication that might or might not have for task groups, though.

[gvanrossum]

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]

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 stacklesslib we 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 CancelledError as special (which is already the case) and not having other exceptions override it.

Implementing a timeout by simply delivering a custom BaseException and 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]

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.

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]

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.

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.

[gvanrossum]

Thanks for the fix, Kristjan. (And welcome back!) I'm backporting this to 3.11 as a bugfix.

[kristjanvalur]

. (And welcome back!) I

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