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

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