GeezerVison

@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)?

@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.

@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 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.

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.

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

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

_must_cancel.

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

@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.

@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.

@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 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.