Asyncio.cancel() a cancellation utility as a coroutine [This time with feeling]

fried · April 29, 2023, 2:25am

I propose asyncio.cancel(task, *, msg=None) a novel utility for helping in cancelling tasks/futures.

Why? I have seen too much code that calls task.cancel() and assumes immediate cancellation. With no await of the cancelled task or even a callback (yuck)

The first thing one needs to do after calling cancel() on the task is to await the task. Which when its finished cancelling will result in a CancelledError being thrown.

We can see in the docs this pattern under Task.cancel

# https://docs.python.org/3/library/asyncio-task.html#asyncio.Task.cancel
task.cancel()
try:
    await task
except asyncio.CancelledError:
    print("...")

BUT… What if the await task took some time and something cancelled that task doing the cancelling? It is going to eat the CancelledError which goes against the contract of cancellation. So we need something a little more complicated to tell the difference between a cancellation from below and one coming from above.

All of this makes task.cancel() pretty non-trivial to use properly. if you need even a short set of steps to do it correctly then we should(imho) have a utility in asyncio that will take care of it for us. And it should be a coroutine so we can await on its completion.

I have an example of this later/task.py at main · facebookincubator/later · GitHub which is what I have had people use @ Meta to insure something is actually cancelled. I’m sure the Asyncio experts can figure a better way to do what I’m doing with shielding but you get the idea. I wrote some tests for that code also later/test_task.py at main · facebookincubator/later · GitHub

The questions come up what about tasks the don’t cancel, that’s “actively discouraged” in the docs. And to keep the implementation simple if you want to wait a limited amount of time you can wrap the call in a Timeout. Also with the new uncancel system i’m sure we could raise an exception if the task you try to cancel is ignoring cancellation.

You could also support the cancel of multiple tasks at one time, but that requires the ExceptionGroup which I didn’t have when I originally wrote later.cancel, it also complicates the code a little bit.

Also I could be completely off my rocker with this idea, but it has been helpful when handling the orderly shutdown of services at Meta without having to rely on asyncio.run to cancel all the outstanding tasks when ordering could be a concern.

fried · April 29, 2023, 2:34am

I also just thought that one could examine current_task to see if we were the ones being cancelled instead of the task below being cancelled.

achimnol · May 1, 2023, 2:17am

github.com/python/cpython

Safe synchronous cancellation in asyncio

opened 07:50PM - 12 Apr 23 UTC

evgeny-osipenko

type-feature topic-asyncio

# Feature or enhancement I propose adding a function to `asyncio` to cancel a… task and then safely wait for the cancellation to complete. The main difficulty with this is deciding whether to swallow or re-raise the `CancelledError` once we catch one. It we call the task that we're waiting on "dependency", and the task doing the waiting "controller", then there are two distinct possibilities: 1. The "dependency" has finished its cleanup, passed its own `CancelledError` all the way up its own stack, and then further up through the `await` in the "controller". In this case, the `CancelledError` should be swallowed, and the "controller" can continue its work normally. 2. The "controller" itself was cancelled, while being on the `await`. In this case, the `CancelledError` is the signal to cancel the "controller" itself, and should be either re-raised further up, or its swallowing should be accompanied by a call to `uncancel`. [The documentation for `asyncio.Task.cancel`](https://docs.python.org/3/library/asyncio-task.html#asyncio.Task.cancel), in fact, does not make this decision correctly, and thus would be a bug. Copying the example, and changing the names to match this issue's terminology: ```py async def dependency(): print('dependency(): start') try: await asyncio.sleep(3600) # a long or infinite loop except asyncio.CancelledError: print('dependency(): cancel') raise finally: print('dependency(): cleanup') async def controller(): dependency_task = asyncio.create_task(dependency()) await asyncio.sleep(1) dependency_task.cancel() try: await dependency_task # controller itself may be cancelled at this moment except asyncio.CancelledError: print("controller(): dependency is cancelled now") # CancelledError swallowed unconditionally ``` If I'm not missing anything, the correct procedure would look like this: ```py async def controller(): dependency_task = asyncio.create_task(dependency()) await asyncio.sleep(1) dependency_task.cancel() try: await dependency_task except asyncio.CancelledError: print("controller(): dependency is cancelled now") if asyncio.current_task().cancelling() > 0: raise ``` Thus, I propose to make these changes: 1. Introduce a function to `asyncio` or `Task`: ```py async def cancel_and_wait(task, msg=None): task.cancel(msg) try: await task except asyncio.CancelledError: if asyncio.current_task().cancelling() == 0: raise else: return # this is the only non-exceptional return else: raise RuntimeError("Cancelled task did not end with an exception") ``` Having a specialized function would reduce the possibility of someone making this mistake in their code (like the author of the example probably did :) ), and allow the implementation to be changed or improved in the future. One such possible enhancement, for example, could be adding a `repeat` parameter to instruct the function, in case the task uncancels itself, to keep cancelling it again in a loop. 2. In the documentation for `asyncio`, add a warning for this kind of mistake, in the "Task Cancellation" section or in the description of `asyncio.Task.cancel`. 3. Change the code example for `asyncio.Task.cancel` to account for cancellation of `main`. I know that, in this specific snippet, it is impossible for `main` itself to be cancelled; but a developer unsuspecting of this issue may copy this example into a situation where the controller is indeed cancellable, and end up with a bug in their code.

I think your argument exactly matches with the above GitHub issue.

async def cancel_and_wait(task, msg=None):
    task.cancel(msg)
    try:
        await task
    except asyncio.CancelledError:
        if asyncio.current_task().cancelling() == 0:
            raise
        else:
            return # this is the only non-exceptional return
    else:
        raise RuntimeError("Cancelled task did not end with an exception")

This seems to be a pretty much repeating pattern along with structured concurrency.
Personally I agree with you in that it would be better to have an instrinsic support for this pattern from the stdlib, but we need to persuade other core developers.

guido · May 8, 2023, 10:32pm

Interesting. I think we need to have someone champion a PEP about this topic so we can get sufficient review of the proposed design. (I can sponsor a PEP but I don’t think I have time to do the writing or coding or research.)

Some suggestions:

cancel_and_await() should return None once the task is cancelled, right? How to distinguish between that and “it was already done”? Maybe the answer is “if you need to know, check task.done() yourself”.
What if it raised some other exception? Probably it should re-raise that.

Hopefully we won’t need a way to cancel multiple tasks – you can just do

async with asyncio.TaskGroup() as tg:
    tg.create_task(cancel_and_wait(t1))
    tg.create_task(cancel_and_wait(t2))

The latest hotness to distinguish between the awaited task exiting with a CancelledError and the awaiting task being cancelled is to use some combination of Task.uncancel() and/or Task.cancelling() – see taskgroups.py and timeouts.py for examples.
The best practice idiom should probably wrap everything in a timeout() call, but that’s up to the caller.

What did I miss?

hels15 · September 1, 2023, 4:08pm

Well, based on the things that were mentioned above, I can open a PR, and maybe a corresponding issue.
From my point of view, it shouldn’t be too complicated and adding this as a pep sounds too much for me(tell me if I’m wrong).

try:
    async with asyncio.timeout(-1):
        await task
except TimeoutError:
     ...

Following the recommended design above, it could be implemented easily.

guido · September 1, 2023, 6:25pm

If the PR is going to be based on @fried’s later library, I would prefer it if @friend himself were to submit the PR – that way proper credit is received and the provenance of the code in the PR is not in question.

@tamas If you want to help, please go over the @fried’s code and see if you understand every step of it completely. Then explain the design here. Once we have a clear understanding we can see if we end up with consensus that that is the right design. Then we can start with the PR. (And let’s be clear that such a PR should also contain unit tests and docs.)

But until we have consensus about the design I don’t see much of a point in opening an issue or PR.

I personally find @fried’s code too complex to try to understand on my day off. E.g. why use shield()?

guido · September 1, 2023, 6:49pm

Moreover there’s the alternative proposal by @achimnol: Safe synchronous cancellation in asyncio · Issue #103486 · python/cpython · GitHub. It would be nice to compare and contrast with @fried’s code.

hels15 · September 1, 2023, 7:26pm

okay, I will try to understand the code, and maybe I can come up with something.

hels15 · September 3, 2023, 3:52pm

I feel like it’s really hard to understand fried’s code without asking him about the purposes of different features being implemented in his function. If he doesn’t want to help anymore or is not active, probably easier to build it from scratch without relying on his code.

hels15 · September 3, 2023, 4:28pm

We also should consider this design:

try:
    async with asyncio.timeout(-1):
        await task
except TimeoutError:
     ...