gh-108951: add TaskGroup.cancel()

[belm0]

Short-circuiting of task groups is a very common, useful, and normal, so make it a first-class operation. The recommended approach to date-- creating a task just to raise an exception, and then catch and suppress the exception-- is inefficient, prone to races, and requires a lot of boilerplate.

• Issue: Add an asyncio.TaskGroup.cancel method #108951

---

📚 Documentation preview 📚: https://cpython-previews--127214.org.readthedocs.build/

[sobolevn]

Thank you! This is not a full review, just a couple of questions.

[sobolevn]

Maybe these tasks should look like this?

async def task(results, num):
    results.append(num)
    await asyncio.sleep(math.inf)
    results.append(-num)

[sobolevn]

So we can assert what was in results

[belm0]

For this particular test, I chose a different test approach, which is to wrap in asyncio.timeout().

For the other tests using count, I'm not sure it's much value, since the test code is only a few lines and there is only one possible path through it. So count result of 0, 1, or 2 each have deterministic meaning that's obvious by looking at the code.

[1st1]

Why call it TaskGroup.stop() and not TaskGroup.cancel()? I'd be more in favor of the latter name.

Short-circuiting of task groups is a very common, useful, and normal, so make it a first-class operation.

Any evidence of this statement? I'd like you to write up technical motivation + examples. That will be useful for the docs.

And speaking of the documentation, you should also show some recipies of how this would be used. Like are you supposed to use this API from within the task group async with clause? Or can you pass the task group to some remote task?

I haven't reviewed the actual implementation in detail yet.

[bedevere-app]

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

[arthur-tacca]

This doesn't work in the case that the body of the task group throws an exception, as in this code:

    async def test_taskgroup_throw_inside(self):

        class MyError(RuntimeError):
            pass

        should_get_here = False
        try:
            async with asyncio.TaskGroup() as tg:
                tg.create_task(asyncio.sleep(0.1))
                tg.stop()
                self.assertEqual(asyncio.current_task().cancelling(), 1)
                raise MyError
            self.fail()  # <-- reaches here instead of raising ExceptionGroup([MyError()])
        except* MyError:
            self.assertEqual(asyncio.current_task().cancelling(), 0)
            should_get_here = True
        self.assertTrue(should_get_here)

The problem is that the new code in the _aexit() method, if not self._errors: return True, is essentially duplicating the if self._errors test later in the function, but in between self._errors is changed by these two lines:

        if et is not None and not issubclass(et, exceptions.CancelledError):
            self._errors.append(exc)

One option is move these lines earlier, before the if self._parent_cancel_requested statement. Then both tests are checking the same thing. This seems to work.

I'd still suggest my original proposal (see the issue) where you just add a single line return True to the very end of _exit() instead of these changes. This avoids duplicating the test in the first place and avoids changing the control flow and, personally, I find it easier to follow.

As a separate point, I'd suggest that the tests could do with a few more checks that asyncio.current_task().cancelling() is correct, like the ones in the test above in this comment.

[belm0]

@1st1

Why call it TaskGroup.stop() and not TaskGroup.cancel()? I'd be more in favor of the latter name.

I'd also prefer cancel(), but per Guido it would be confusing since such a method would be expected to raise CancelledError, and he suggested stop().

Short-circuiting of task groups is a very common, useful, and normal, so make it a first-class operation.

Any evidence of this statement? I'd like you to write up technical motivation + examples. That will be useful for the docs.

In trio the equivalent is nursery.cancel_scope.cancel(), which has > 1,000 hits on github, despite the unpopularity of trio.

I have years experience developing a non-trivial, production async app, which I've presented at PyCon JP. Anecdotally, I can't imagine how painful and unproductive it would be to not have short circuiting of task groups.

And speaking of the documentation, you should also show some recipies of how this would be used. Like are you supposed to use this API from within the task group async with clause? Or can you pass the task group to some remote task?

All is on the table: stop from within the TaskGroup body, from a child, from some other entity you've passed the bound stop() method to.

[smurfix]

I'd also prefer cancel(), but per Guido it would be confusing since such a method would be expected to raise CancelledError,

Well, that's exactly what it does, isn't it? The fact that the cancelled taskgroup catches the CancelledErrors raised by itself doesn't change that. You don't get to wait on taskgroups the way you wait on tasks, thus the exception isn't visible like when you await on a cancelled task, but that's a minor detail IMHO.

Also, trio and anyio already call this operation cancel.

[graingert]

Probably you want code examples for all of these?

[graingert]

what do you think about supporting cancel messages here?

[belm0]

I asked on Gitter, but I'm still unclear about how such a message would be accessed and surfaced.

[graingert]

It could be logged by the task that gets cancelled, or useful in debugging

[asvetlov]

I would keep it as-is and maybe add a message in the follow-up PR; this PR is big enough for the review.

[gvanrossum]

I asked on Gitter, but I'm still unclear about how such a message would be accessed and surfaced.

My $0.02:

1. Assuming that message gets passed into each task, indeed, those tasks can do something with it (like identifying who cancelled it -- this is a private protocol within an app or library).
2. If we end up raising CancelledError out of the async with block, the same is true for whoever catches that CancelledError.

[gvanrossum]

IOW, in case I wasn't clear, yes, we should add cancel message support. (Even if in the end we renamed the method to stop() -- it'll still call .cancel() on many tasks that might want to participate in such a private protocol.

[belm0]

I think having a cancel message is going to encourage some anti-patterns such as parsing the string to determine code flow (this is why we stopped raising bare strings). If I were reviewing code using a message this way, I'd ask to refactor the code to signal "I cancelled you" in some more direct way.

My sense is that this is much lower priority, and can always be reconsidered in the future.

[gvanrossum]

Nevertheless it's a feature that most cancellable objects follow. And I don't think that anti-pattern has surfaced in practice. Why would TaskGroup be different in this respect? It is a feature third-party code uses, as a private protocol. Note that we don't even enforce that it is a string -- it can be anything, since it's just getting passed around.

For an implementation example, see Future.cancel.

[graingert]

can you test with eager tasks as well as regular tasks?

I think something like this:

class TestTaskGroupLazy(IsolatedAsyncioTestCase):
    loop_factory = asyncio.EventLoop


class TestTaskGroupEager(TestTaskGroupLazy):
    @staticmethod
    def loop_factory():
        loop = asyncio.EventLoop()
        loop.set_task_factory(asyncio.eager_task_factory)
        return loop

if you find the existing tests fail in eager tasks then probably just add the eager tests for your newly added tests.

[belm0]

I asked on Gitter, but I'm still unclear about how such a message would be accessed and surfaced.

[github-actions]

This PR is stale because it has been open for 30 days with no activity.

[gvanrossum]

I have some comments. This is not an endorsement of this PR -- I want to look at @smurfix' version too before deciding. I'm also still torn between naming it stop() or cancel().

Edit: There is no PR by @smurfix -- he opened the issue. Sorry. I think that with the feedback (mine and others') addressed and a decision on the name (stop or cancel) made we can merge this in time before the May 1st beta deadline (in ~10 days).

[gvanrossum]

IOW, in case I wasn't clear, yes, we should add cancel message support. (Even if in the end we renamed the method to stop() -- it'll still call .cancel() on many tasks that might want to participate in such a private protocol.

[gvanrossum]

For email readers: Sorry; there is no PR by @smurfix -- he opened the issue. I think that with the feedback (mine and others') addressed (possibly by explaining why no change is needed!) and a decision on the name (stop or cancel) made we can merge this in time before the May 1st beta deadline (in ~10 days).

[gvanrossum]

I count the following TODOs:

• Decide on name: stop vs. cancel (it feels odd that something called cancel() ultimately suppresses the CanceledException)
• Cancel messages (though this could be added in a post-beta-1 bugfix)
• Test comments

[gvanrossum]

Nevertheless it's a feature that most cancellable objects follow. And I don't think that anti-pattern has surfaced in practice. Why would TaskGroup be different in this respect? It is a feature third-party code uses, as a private protocol. Note that we don't even enforce that it is a string -- it can be anything, since it's just getting passed around.

For an implementation example, see Future.cancel.

[gvanrossum]

Also there are merge conflicts. :-(

[gvanrossum]

I did the merge and fixed the stray quote in NEWS.

[gvanrossum]

[@1st1 / Yury -- on Nov 25, 2024]

Why call it TaskGroup.stop() and not TaskGroup.cancel()? I'd be more in favor of the latter name.

Hm. If Yury's intuition goes towards cancel(), I revoke my opposition to it. Decision made: it's cancel().

Short-circuiting of task groups is a very common, useful, and normal, so make it a first-class operation.

Any evidence of this statement? I'd like you to write up technical motivation + examples. That will be useful for the docs.

The main evidence is the huge paragraph that was added to the 3.12/3.13/3.14 docs about how to terminate a TaskGroup.

And speaking of the documentation, you should also show some recipes of how this would be used. Like are you supposed to use this API from within the task group async with clause? Or can you pass the task group to some remote task?

All of the above. Whoever has a reference to a TaskGroup can call its cancel() and the right thing will happen.

I haven't reviewed the actual implementation in detail yet.

I have and it's fine.

[gvanrossum]

A failing test complains about deleting a label ("Terminating a Task Group"):

The above HTML IDs were removed from the documentation, resulting in broken links. Please add them back.
library/asyncio-task.html: terminating-a-task-group

Alternatively, add them to Doc/tools/removed-ids.txt.

I think the latter is best -- though you could also bring the label back with only the body text (paraphrased):

Use the new cancel() method. Or refer to the 3.14 docs to do it in a backwards-compatible way.