gh-116720: Fix corner cases of taskgroups by gvanrossum · Pull Request #117407 · python/cpython

gvanrossum · 2024-03-31T16:00:52Z

Make test_timeouts pass (test_timeout_taskgroup)
Add tests derives from examples in the issue
Add test for new uncancel() behavior (reset _must_cancel)
Add news entry
Add What's New entry
Update docs for uncancel()
Update docs for task groups
Add Co-Authored-By: @arthur-tacca

Issue: Nested TaskGroup can silently swallow cancellation request from parent TaskGroup #116720

gvanrossum · 2024-04-02T17:39:24Z

@arthur-tacca, what do you think of this PR? Does it pass all your tests? Do you want to contribute in any way (e.g. by converting your example programs into unittests)?

gvanrossum · 2024-04-02T17:42:03Z

@agronholm I wonder if you could review this PR? (It's in draft mode because there are no tests or doc yet, but the changes to taskgroups.py and tasks.py are as I plan to keep them.)

It might affect anyio because of two things: (a) Better support for nested taskgroups; (b) change to uncancel() to reset the internal "must cancel" flag when the pending cancellation count reaches zero.

gvanrossum · 2024-04-02T17:44:13Z

@Tinche Same question for you -- you might have an opinion on the changes that I'm proposing to make to asyncio task and task group behavior here.

agronholm · 2024-04-02T19:00:09Z

@agronholm I wonder if you could review this PR? (It's in draft mode because there are no tests or doc yet, but the changes to taskgroups.py and tasks.py are as I plan to keep them.)

It might affect anyio because of two things: (a) Better support for nested taskgroups; (b) change to uncancel() to reset the internal "must cancel" flag when the pending cancellation count reaches zero.

Sure, I can take a look.

agronholm · 2024-04-02T21:20:53Z

It's hard to definitively say if this will affect AnyIO, but I did run the task group tests against this PR and they passed. The only place where AnyIO looks at this flag is in CancelScope._deliver_cancellation() where it will skip explicitly cancelling a task if this flag is already set.

gvanrossum · 2024-04-02T21:24:22Z

Sorry, which flag is that? ‘cancelling()’ or ‘_must_cancel’?

agronholm · 2024-04-02T21:26:00Z

Sorry, which flag is that? ‘cancelling()’ or ‘_must_cancel’?

_must_cancel.

agronholm · 2024-04-02T21:24:41Z

Lib/asyncio/tasks.py

        if self._num_cancels_requested > 0:
            self._num_cancels_requested -= 1
+            if self._num_cancels_requested == 0:
+                self._must_cancel = False


Shouldn't this also clear the cancel message?

I doubt it -- self._cancel_message is never cleared, and it's never used unless self._must_cancel is set.

Okay – I was just confused by that Py_CLEAR() in the C version, but this explains it.

Oh, you're right, that CLEAR is unnecessary there too. :-)

This fixes the one failing test in test_timeouts. Surprisingly, it doesn't break any other asyncio tests.

@Tinche

Co-Authored-By: @Tinche

@arthur-tacca

Co-Authored-By: @arthur-tacca

@arthur-tacca

Co-Authored-By: @arthur-tacca

gvanrossum · 2024-04-04T23:07:30Z

Still to do are docs updates for uncancel and for task groups. Also, I'd love help with shortening the news blurb.

gvanrossum · 2024-04-05T22:22:56Z

@willingc Would you be my reviewer for this PR? I feel particularly challenged by the various documentation updates -- this is a pretty subtle improvement (given how long it's taken before someone reported the issue :-) and I'm struggling describing the changes and the end state appropriately in various places:

Changelog (Misc/NEWS)
What's new in 3.13
asyncio-task.rst: uncancel docs and task group docs

I'd say that currently the Changelog news item is probably too long, and there's too much duplication between all three forms of documentation.

@arthur-tacca: I haven't heard from you on the issue in a while. Your feedback on any part of this PR would be much appreciated. I plan to give you credit by adding Co-Authored-By: Arthur Tacca to the final commit message, since I cribbed most of the unit tests from your original demonstrations. I will also add (Inspired by an issue reported by Arthur Tacca in :gh:`116720`.) to the "what's new" section.

arthur-tacca · 2024-04-07T16:01:53Z

@gvanrossum Sorry for ghosting you. I had some limited time for open source between jobs (which I actually planned to spend mainly on something else), which has run out and it's now hard for me to post even short comments like this one.

Super-brief review: Your code changes look good to me. As I said in an earlier comment, I think it's good that you've just moved (rather than duplicated) the if self._parent_cancel_requested check. The check for self._parent_task.cancelling() also looks good. As for the change to Task (uncancel() will rest _must_cancel if cancelling reaches 0), I can't immediately think of a situation it would naturally arise when using task groups (as opposed to manually triggering it with an explicit uncancel() call) and I don't have time to try and figure out one, but it looks like a sensible thing to do if that situation does arise; perhaps it would come up with asyncio.Timeout objects, which I haven't experimented with at all.

Credit: Thanks for crediting me, that's much appreciated. I don't think my contributions are significant enough to need a CLA but in any case I have signed one (for a small change to typing extensions).

Changelog:

"... For example, when two task groups are nested and both experience an exception in a child task simultaneously, it was possible that the outer task group would hang, because ..." I'm not sure "would hang" is accurate. It would process the rest of the logic after the inner task group.
"This allows a try / except* surrounding the task group to handle the exceptions in the ExceptionGroup without losing the cancellation." This is a bit specific to the particular example I gave. For a start, it also applies to try / finally. It also could stop a except* or finally block running in full (if it has await ... in it), which is sort of the opposite of what the text says. Perhaps better to say something deliberately a bit less specific, e.g. more along the lines of "This ensures that a CancelledError will be raised at the next await, so the cancellation is not lost".

Docs changes: "Task groups are careful not to drop outside cancellations when they collide with a cancellation internal to the task group." I must admit, I would struggle to understand what this means if I hadn't been involved in this issue. But that isn't very helpful since I don't have a better suggestion to give.

willingc · 2024-04-07T16:20:05Z

Lib/asyncio/taskgroups.py

            self._errors.append(exc)

        if self._errors:
+            # If the parent task is being cancelled from the outside,


Suggested change

# If the parent task is being cancelled from the outside,

# If the parent task is being cancelled from the outside of the taskgroup,

willingc

@gvanrossum Overall, this looks good. I made a few suggestions on wording, but the existing text is also fine. Thanks!

willingc · 2024-04-07T16:21:54Z

Lib/asyncio/taskgroups.py


        if self._errors:
+            # If the parent task is being cancelled from the outside,
+            # re-cancel it, while keeping the cancel count stable.


Suggested change

# re-cancel it, while keeping the cancel count stable.

# un-cancel and re-cancel the parent task, which will keep the cancel count stable.

willingc · 2024-04-07T16:30:50Z

Doc/library/asyncio-task.rst

      the cancellation state.

+      When this method decrements the cancellation count to zero,
+      if a previous :meth:`cancel` call had arranged for a


Suggested change

if a previous :meth:`cancel` call had arranged for a

the method checks if a previous :meth:`cancel` call had arranged for

willingc · 2024-04-07T16:31:05Z

Doc/library/asyncio-task.rst


+      When this method decrements the cancellation count to zero,
+      if a previous :meth:`cancel` call had arranged for a
+      :exc:`CancelledError` to be thrown into the task,


Suggested change

:exc:`CancelledError` to be thrown into the task,

:exc:`CancelledError` to be thrown into the task.

willingc · 2024-04-07T16:32:11Z

Doc/library/asyncio-task.rst

+      When this method decrements the cancellation count to zero,
+      if a previous :meth:`cancel` call had arranged for a
+      :exc:`CancelledError` to be thrown into the task,
+      but this hadn't been done yet, that arrangement will be


Suggested change

but this hadn't been done yet, that arrangement will be

If this hadn't been done yet, that arrangement will be

willingc · 2024-04-07T16:32:44Z

Doc/whatsnew/3.13.rst

+  (Contributed by Arthur Tacca and Jason Zhang in :gh:`115957`.)
+
+* Improved behavior of :class:`asyncio.TaskGroup` when an outside cancellation
+  collides with an internal cancellation. For example, when two task groups


Also switch to consistently using internal/external instead of inside/outside.

willingc

Looks good. Thanks everyone.

Tinche · 2024-04-16T21:47:41Z

@gvanrossum would you mind if I copied your solution into quattro to serve as a backport of sorts, for 3.11 and 3.12? Quattro is Apache 2 licensed, that's compatible right?

gvanrossum · 2024-04-16T22:40:32Z

@gvanrossum would you mind if I copied your solution into quattro to serve as a backport of sorts, for 3.11 and 3.12? Quattro is Apache 2 licensed, that's compatible right?

That should be absolutely fine -- if I understand the PSF license correctly, all you need to do is add this text somewhere

Copyright © 2001-2024 Python Software Foundation; All Rights Reserved

and add a copy of the PSF license for 3.13 somewhere.

This prevents external cancellations of a task group's parent task to be dropped when an internal cancellation happens at the same time. Also strengthen the semantics of uncancel() to clear self._must_cancel when the cancellation count reaches zero. Co-Authored-By: Tin Tvrtković <tinchester@gmail.com> Co-Authored-By: Arthur Tacca

bedevere-app bot mentioned this pull request Mar 31, 2024

Nested TaskGroup can silently swallow cancellation request from parent TaskGroup #116720

Closed

gvanrossum force-pushed the fix-tg branch 2 times, most recently from e97f22a to 5912d70 Compare April 2, 2024 17:37

agronholm reviewed Apr 2, 2024

View reviewed changes

gvanrossum added 8 commits April 4, 2024 09:23

Fix taskgroups handling of parent cancellation

90695c5

When errors and parent cancellation are both pending, re-cancel parent

6c4876d

When uncancel() reaches zero, clear _must_cancel

df6e8b7

This fixes the one failing test in test_timeouts. Surprisingly, it doesn't break any other asyncio tests.

Remove 'experimental' comment

c762e36

Remove unneeded Py_CLEAR(self->task_cancel_msg)

4989042

Simple test for preserving cancelling() level

3f91f2d

Co-Authored-By: @Tinche

Add test for nested task groups

55ff73f

Co-Authored-By: @arthur-tacca

Add test that is fixed by the re-cancel in __aexit__

50fd8d6

Co-Authored-By: @arthur-tacca

gvanrossum force-pushed the fix-tg branch from 35c807b to 50fd8d6 Compare April 4, 2024 19:55

gvanrossum added 4 commits April 4, 2024 14:57

Test that uncancel() sets _must_cancel to False

93ba76a

Move a stray asyncio news item into the asyncio section

f1f89c0

Add blurb

48c6dda

Add what's new entry

1e20728

gvanrossum added 4 commits April 5, 2024 15:03

Update uncancel docs

bc1522c

Update task group docs

7f6e5ff

Merge remote-tracking branch 'origin/main' into fix-tg

56251ff

Fix markup error

c54fb22

gvanrossum marked this pull request as ready for review April 5, 2024 22:09

gvanrossum requested review from 1st1, asvetlov, kumaraditya303 and willingc as code owners April 5, 2024 22:09

gvanrossum self-assigned this Apr 5, 2024

bedevere-app bot added the awaiting core review label Apr 5, 2024

gvanrossum requested a review from agronholm April 5, 2024 22:09

Add attribution to Arthur Tacca issue

6d4b3d7

willingc reviewed Apr 7, 2024

View reviewed changes

willingc approved these changes Apr 7, 2024

View reviewed changes

bedevere-app bot added awaiting merge and removed awaiting core review labels Apr 7, 2024

gvanrossum added 4 commits April 8, 2024 13:41

Wording changes suggested by Carol Willing

b086609

Changelog/whatsnew updates from Arthur Tacca

8ac42f6

Also switch to consistently using internal/external instead of inside/outside.

Attempted doc improvements inspired by Arthur Tacca's feedback

ac87a8e

Merge remote-tracking branch 'origin/main' into fix-tg

be9365c

gvanrossum requested a review from willingc April 9, 2024 00:17

willingc approved these changes Apr 9, 2024

View reviewed changes

gvanrossum merged commit fa58e75 into python:main Apr 9, 2024

bedevere-app bot removed the awaiting merge label Apr 9, 2024

gvanrossum deleted the fix-tg branch April 9, 2024 15:17

	# If the parent task is being cancelled from the outside,
	# If the parent task is being cancelled from the outside of the taskgroup,

	# re-cancel it, while keeping the cancel count stable.
	# un-cancel and re-cancel the parent task, which will keep the cancel count stable.

	if a previous :meth:`cancel` call had arranged for a
	the method checks if a previous :meth:`cancel` call had arranged for

	:exc:`CancelledError` to be thrown into the task,
	:exc:`CancelledError` to be thrown into the task.

	but this hadn't been done yet, that arrangement will be
	If this hadn't been done yet, that arrangement will be

Uh oh!

Conversation

gvanrossum commented Mar 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gvanrossum commented Apr 2, 2024

Uh oh!

gvanrossum commented Apr 2, 2024

Uh oh!

gvanrossum commented Apr 2, 2024

Uh oh!

agronholm commented Apr 2, 2024

Uh oh!

agronholm commented Apr 2, 2024

Uh oh!

gvanrossum commented Apr 2, 2024

Uh oh!

agronholm commented Apr 2, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gvanrossum commented Apr 4, 2024

Uh oh!

gvanrossum commented Apr 5, 2024

Uh oh!

arthur-tacca commented Apr 7, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

willingc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

willingc Apr 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

willingc left a comment

Choose a reason for hiding this comment

Uh oh!

Tinche commented Apr 16, 2024

Uh oh!

gvanrossum commented Apr 16, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

gvanrossum commented Mar 31, 2024 •

edited

Loading

willingc Apr 7, 2024 •

edited

Loading