It seems like the future subclass returned by asyncio.gather() (_GatheringFuture) can never return True for future.cancelled() even after it's cancel() has been invoked successfully (returning True) and an await on it actually raised a CancelledError. This is in contrast to the behavior of normal Futures and it seems generally to be classified as a minor bug by developers.

* Stackoverflow Post: https://stackoverflow.com/questions/61942306/asyncio-gather-task-cancelled-is-false-after-task-cancel
* Github snippet: https://gist.github.com/timmwagener/dfed038dc2081c8b5a770e175ba3756b

I have created a fix and will create a PR. It seemed rather easy to fix and the asyncio test suite still succeeds. So maybe this is a minor bug, whose fix has no backward-compatibility consequences. However, my understanding is that asyncio.gather() is scheduled for deprecation, so maybe changes in this area are on halt anyways!?

----

# excerpt from snippet
async def main():
    """Cancel a gather() future and child and return it."""

    task_child = ensure_future(sleep(1.0))
    future_gather = gather(task_child)

    future_gather.cancel()
    try:
        await future_gather
    except CancelledError:
        pass

    return future_gather, task_child


# run
future_gather, task_child = run(main())

# log gather state
logger.info(future_gather.cancelled())  # False / UNEXPECTED / ASSUMED BUG
logger.info(future_gather.done())  # True
logger.info(future_gather.exception())  # CancelledError
logger.info(future_gather._state)  # FINISHED

# log child state
logger.info(task_child.cancelled())  # True
logger.info(task_child.done())  # True
# logger.info(task_child.exception())  Raises because _state is CANCELLED
logger.info(task_child._state)  # CANCELLED

Thank you for the bug report Timm.

While I can certainly understand the source of the confusion here, I think you may be misunderstanding an important part of cancellation for futures. Specifically a future can't be cancelled once it reaches the PENDING state, this is indicated when future.cancel() returns false; see https://github.com/python/cpython/blob/0e96c419d7287c3c7f155c9f2de3c61020386256/Lib/asyncio/futures.py#L152 for the source code. In your example code snippet, you can also see this if you replace the line:

```
future_gather.cancel()
```
with the following:
```
if not future_gather.cancel():
    logger.info("future_gather could not be cancelled")
```

Also, to clarify on something else:
> However, my understanding is that asyncio.gather() is scheduled for deprecation, so maybe changes in this area are on halt anyways!?

asyncio.gather() is not deprecated or scheduled for deprecation, it is simply the loop argument being deprecated.

> Specifically a future can't be cancelled once it reaches the PENDING state, this is indicated when future.cancel() returns false; see https://github.com/python/cpython/blob/0e96c419d7287c3c7f155c9f2de3c61020386256/Lib/asyncio/futures.py#L152 for the source code

Actually, I was mistaken in my previous message; sorry about that. It can *only* be cancelled when the state is PENDING. So, I suspect there's a race condition where the state is `PENDING` when future.cancel() is called, but transitions to `FINISHED` before the future is actually cancelled. I'll insert some logging in `future.cancel()` to verify this locally.

The approach in PR-20686 has a fundamental problem though: simply because cancellation was requested does not mean the future was cancelled. So, we can't rely on checking ``self.done() and self._cancel_requested`` for future.cancelled() as this would mean that future.cancelled() would return true for a future that fully completed if `future.cancel()` was called after it finished (which is incorrect).

I'll have to do some further investigation to determine the cause and a potential fix of the race condition. Thanks again for reporting it, and my apologies for the earlier mixup.

> So, we can't rely on checking ``self.done() and self._cancel_requested`` for future.cancelled() as this would mean that future.cancelled() would return true for a future that fully completed if `future.cancel()` was called after it finished (which is incorrect).

This should be "was called *as* it finished", not "was called *after* it was finished". If gather_future.cancel() is called after the future is `FINISHED`, it will immediately return false since it checks `self.done()` first (https://github.com/python/cpython/blob/b8f67c0923ac85468dfbfd43375e0bbfb6ca50ea/Lib/asyncio/tasks.py#L727).

Upon further investigation, I've realized that the issue is just that the cancel() override for `_GatheringFuture` never sets its state to CANCELLED at any point (unlike its parent), and is instead going to always be set to FINISHED because of the `outer.set_exception()` (https://github.com/python/cpython/blob/b8f67c0923ac85468dfbfd43375e0bbfb6ca50ea/Lib/asyncio/tasks.py#L826 or https://github.com/python/cpython/blob/b8f67c0923ac85468dfbfd43375e0bbfb6ca50ea/Lib/asyncio/tasks.py#L791, depending on the arg for *return_exceptions*). 

Since we are unable to set the state of `_GatheringFuture` (at least not without causing side effects), I'm starting to agree that it makes sense to override the behavior for `cancelled()`. However, it might make more sense to replace the `self._cancel_requested` check with  `isinstance(self.exception(), exceptions.CancelledError)`, as this more accurately indicates if and when the gather() was cancelled. I don't think we should rely on `self._cancel_requested` since it would be true before the future is actually cancelled and not always be correct since the gather() can be cancelled without setting `self._cancel_requested`.

Here's one case where relying on `self._cancel_requested` for future_gather.cancelled() wouldn't work, based on a slight modification of the reproducer example:

```
async def main():
    """Cancel a gather() future and child and return it."""
    task_child = ensure_future(sleep(1.0))
    future_gather = gather(task_child)

    task_child.cancel()
    try:
        await future_gather
    except asyncio.CancelledError:
        pass

    return future_gather, task_child
```

(Despite `await future_gather` raising a CancelError and effectively being cancelled, `future_gather.cancelled()` would return false since `self._cancel_requested` was never set to true.)

Hello Kyle, thanks for reviewing.

> I'm starting to agree that it makes sense to override the behavior for `cancelled()`. However, it might make more sense to replace the `self._cancel_requested` check with  `isinstance(self.exception(), exceptions.CancelledError)`, as this more accurately indicates if and when the gather() was cancelled. I don't think we should rely on `self._cancel_requested` since it would be true before the future is actually cancelled and not always be correct since the gather() can be cancelled without setting `self._cancel_requested`.

My understanding of the current logic is, that there is a distinction between an explicit `gather().cancel()` and an external `child.cancel()`. This distinction is made by the `_cancel_requested` variable:

* **Explicit gather.cancel():** Only an explicit cancellation of the gather task should result in `cancelled() is True`. This explicit cancellation is indicated by `_cancel_requested`. It works regardless of the `return_exceptions` kwarg. value. A `CancelledError` is always raised at the `await` site and `cancelled()` is True, provided that a previous `gather.cancel()` has been `True`.

* **External invocation of child.cancel()/Implicit cancellation, finishing of gather:** In this case, no explicit user intent has caused a cancellation of gather. This is reflected by `_cancel_requested` being `False` and `gather.cancelled()` always being `False`. Instead based on the value of the `return_exceptions` kwarg.:
  
  * **False:** The normal `set_exception()` is invoked and the child exception is set on the gather future _(in this case a `CancelledError`)_ while also setting state to `FINISHED`. This results in the gather raising at the `await` site and finishing. What makes it slighty confusing though is the exception being a `CancelledError`. However, i believe its otherwise in line with how `Future.cancelled()` behaves for any exception.
  
  * **True:** `CancelledErrors` in children are collected and returned amongst other results and exceptions in a list.

> Here's one case where relying on `self._cancel_requested` for future_gather.cancelled() wouldn't work, based on a slight modification of the reproducer example:

As outlined above, i'd assume this falls into the category of _implicit cancellation, finishing of gather without user intent_ for which I'm assuming `cancelled()` should be `False`. However, as mentioned, this is just my assumption based on the logic. One could also take your viewpoint, that `cancelled()` should be `True` when `fut.exception()` is a `CancelledError`.

> This should be "was called *as* it finished", not "was called *after* it was finished". If gather_future.cancel() is called after the future is `FINISHED`, it will immediately return false since it checks `self.done()` first

I assume that these scenarios apply, when this like `current_task().cancel()` is happening in a `done()` callback or such? It seems like many (corner)-cases can be created and i'm certainly not aware of them. However, as `gather()` adds it's own callback first to its passed futures, and `outer` is not available beforehand for any done callback registration, i'm not sure how much of an effort one would have to make to create the case you described. But maybe I also didn't really understand the point you are getting at.

> asyncio.gather() is not deprecated or scheduled for deprecation, it is simply the loop argument being deprecated.

I was actually referring to [this PR comment](https://github.com/python/cpython/pull/6694#issuecomment-543445208) , which I happened to stumble upon while researching for the issue.
> We'll likely have TaskGroups in asyncio 3.9 and will simply deprecate asyncio.gather.

That's why I mentioned in the beginning that it may be a small, easy correctness improvement for a Python patch version if it's backwards compatible and not causing too much trouble. But maybe its also not considered that important anymore. I'd leave that up to the reviewers ;)

TLDR;
-----
The intention of the PR is to make a future from gather return that cancelled() is True if, and only if, cancel() has successfully been called on it (explicit user intent) and it was awaited/has finished. All other finishing is not considered explicit user intent and has the state FINISHED (with exception or result), even if the exception is a CancelledError.

As mentioned, just my interpretation based on the logic i've seen.

Kyle is correct.  By analogy with Kyle's example, the following example has no gather, only two nested futures:

```
# childfut.py
import asyncio

async def f(fut):
    await fut

async def g(t):
    await asyncio.sleep(t)

async def main():
    fut_g = asyncio.create_task(g(1))
    fut_f = asyncio.create_task(f(fut_g))

    try:

        # Cancel the "child" future
        fut_g.cancel()

        await fut_f
    except asyncio.CancelledError as e:
        pass

    print(f'fut_f done? {fut_f.done()} fut_f cancelled? {fut_f.cancelled()}')
    print(f'fut_g done? {fut_g.done()} fut_g cancelled? {fut_g.cancelled()}')

asyncio.run(main())
```

It produces:

```
$ python childfut.py
fut_f done? True fut_f cancelled? True
fut_g done? True fut_g cancelled? True
```

The outer future f, has f.cancelled() == True even though it was the inner future got cancelled.

I think `gather()` should work the same. It would be confusing if `future_gather.cancelled()` is false if a child is cancelled, while a plain old outer future returns `future.cancelled() == true` if futures that it waits on are cancelled.

Thanks for the additional feedback, Caleb.

> I think `gather()` should work the same. It would be confusing if `future_gather.cancelled()` is false if a child is cancelled, while a plain old outer future returns `future.cancelled() == true` if futures that it waits on are cancelled.

Agreed, I think it would make the most sense IMO for the behavior of .cancelled() to be similar for tasks, futures, and for gather itself. So, I'm still inclined towards checking the exception to see if CancelledError occurred for _GatheringFuture.cancelled(), as I think that would result in the most similar behavior.

Currently waiting on feedback from Yury and/or Andrew about the current long-term plans for asyncio.gather(), and their general thoughts on this issue. Unless it is realistically going to be deprecated and effectively replaced with task groups in 3.10, I think the change would make sense.

Ok, seems reasonable as well. I'll change it as proposed.

Proposed changes are done and test coverage is extended. I had to change one previous test slightly *(test_one_cancellation())*, which seems in line though, with the proposed behavior.

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.