[issue40894] asyncio.gather() cancelled() always False

[Ben Buchwald]

Ben Buchwald <sparkyb at gmail.com> added the comment:

Hopefully I'm not too late to comment on this. I also just hit this issue, but I do not agree with the proposed PR. Only modifying _GatheringFuture.cancelled() just fixes one of the side-effects of the problem. The state of the future is still FINISHED and not CANCELLED and there are still other differences that can be observed due to this. When calling .exception() a cancelled future should *raise* a CancelledError, but with a _GatheringFuture it will return a CancelledError regardless of the value that .cancelled() returns. Also, if you don't fetch the exception of a future, when it gets deleted it will warn you, but this is not supposed to happen with cancellation. This unexpected warning was how I discovered that I have this issue, and the proposed fix doesn't solve my case.

Instead of overriding .cancelled() on _GatheringFuture, gather() should be updated to actually cancel the _GatheringFuture. I suggest that in gather's _done_callback this:

if outer._cancel_requested:
    # If gather is being cancelled we must propagate the
    # cancellation regardless of *return_exceptions* argument.
    # See issue 32684.
    exc = fut._make_cancelled_error()
    outer.set_exception(exc)

Should be changed to:

if outer._cancel_requested:
    # If gather is being cancelled we must propagate the
    # cancellation regardless of *return_exceptions* argument.
    # See issue 32684.
    exc = fut._make_cancelled_error()
    super(_GatheringFuture, other).cancel(fut._cancel_message)

This alone would only fix it in the return_exceptions=True case. To fix it when return_exceptions=False, a bit higher up in the same function, change:

if not return_exceptions:
    if fut.cancelled():
        # Check if 'fut' is cancelled first, as
        # 'fut.exception()' will *raise* a CancelledError
        # instead of returning it.
        exc = fut._make_cancelled_error()
        outer.set_exception(exc)
        return

to:

if not return_exceptions:
    if fut.cancelled():
        # Check if 'fut' is cancelled first, as
        # 'fut.exception()' will *raise* a CancelledError
        # instead of returning it.
        if outer._cancel_requested:
          super(_GatheringFuture, outer).cancel(fut._cancel_message)
        else:
          exc = fut._make_cancelled_error()
          outer.set_exception(exc)
        return

This case is a little trickier. Notice that I added a new if. As Caleb and Kyle pointed out, a task gets cancelled implicitly if its child gets cancelled. To be consistent with that behavior, you wouldn't actually care if _cancel_requested is set, if you'd always just call the super cancel. However, I actually think that gather *should* differ from Task here. A task wraps a single coroutine. If that coroutine is cancelled it makes sense that the task is implicitly cancelled. But with gather, I'm not convinced that cancelling one of the children should apply to the whole gather. It think it makes more sense that one child being cancelled would be treated as an exceptional return of the collection.

The one other thing I'd change is that I'd not actually use fut._cancel_message as I do in those examples above. I'd save the original message in _GatheringFuture.cancel() when setting _cancel_requested, and then use outer._cancel_message. I've attached The whole code I would suggest. I don't have time right now to make my own PR, but since Timm already has an open PR, hopefully this helps.

----------
nosy: +sparkyb
Added file: https://bugs.python.org/file49895/gather.py

_______________________________________
Python tracker <report at bugs.python.org>
<https://bugs.python.org/issue40894>
_______________________________________