asyncio.gather() cancelled() always False

[timmwagener]

BPO	40894
Nosy	@asvetlov, @cjrh, @1st1, @aeros, @timmwagener, @sparkyb
PRs	bpo-40894: asyncio.gather() cancelled() always False #20686
Files	gather.py

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = None
created_at = <Date 2020-06-06.22:13:50.993>
labels = ['type-feature', '3.8', 'expert-asyncio']
title = 'asyncio.gather() cancelled() always False'
updated_at = <Date 2021-03-21.00:05:17.747>
user = 'https://github.com/timmwagener'

bugs.python.org fields:

activity = <Date 2021-03-21.00:05:17.747>
actor = 'sparkyb'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['asyncio']
creation = <Date 2020-06-06.22:13:50.993>
creator = 'timmwagener'
dependencies = []
files = ['49895']
hgrepos = []
issue_num = 40894
keywords = ['patch']
message_count = 12.0
messages = ['370855', '370864', '370867', '370868', '370874', '370886', '370905', '371436', '371439', '371441', '371448', '389202']
nosy_count = 6.0
nosy_names = ['asvetlov', 'cjrh', 'yselivanov', 'aeros', 'timmwagener', 'sparkyb']
pr_nums = ['20686']
priority = 'normal'
resolution = None
stage = 'patch review'
status = 'open'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue40894'
versions = ['Python 3.8']

[timmwagener]

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

[aeros]

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

cpython/Lib/asyncio/futures.py

Line 152 in 0e96c41

if self._state != _PENDING:

for the source code. In your example code snippet, you can also see this if you replace the line:

future_gather.cancel()
``````py
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.

[aeros]

Specifically a future can't be cancelled once it reaches the PENDING state, this is indicated when future.cancel() returns false; see

cpython/Lib/asyncio/futures.py

Line 152 in 0e96c41

if self._state != _PENDING:

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.

[aeros]

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

[aeros]

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

[timmwagener]

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 , 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 ;)

[timmwagener]

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.

[cjrh]

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.

[aeros]

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.

[timmwagener]

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

[timmwagener]

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.

[sparkyb]

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.