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

[Ben Buchwald]

Ben Buchwald  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 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Timm Wagener]

Timm Wagener  added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Timm Wagener]

Timm Wagener  added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Kyle Stanley]

Kyle Stanley  added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Caleb Hattingh]

Caleb Hattingh  added the comment:

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.

--
nosy: +cjrh

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Timm Wagener]

Timm Wagener  added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Timm Wagener]

Timm Wagener  added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Kyle Stanley]

Kyle Stanley  added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Kyle Stanley]

Kyle Stanley  added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Kyle Stanley]

Kyle Stanley  added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Kyle Stanley]

Kyle Stanley  added the comment:

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.

--
nosy: +aeros

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Timm Wagener]

Change by Timm Wagener :


--
keywords: +patch
pull_requests: +19898
stage:  -> patch review
pull_request: https://github.com/python/cpython/pull/20686

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Timm Wagener]

New submission from Timm Wagener :

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

--
components: asyncio
messages: 370855
nosy: asvetlov, timmwagener, yselivanov
priority: normal
severity: normal
status: open
title: asyncio.gather() cancelled() always False
type: enhancement
versions: Python 3.8

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com