This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

Author timmwagener
Recipients aeros, asvetlov, timmwagener, yselivanov
Date 2020-06-07.11:03:33
SpamBayes Score -1.0
Marked as misclassified Yes
Message-id <1591527815.25.0.526507516359.issue40894@roundup.psfhosted.org>
In-reply-to
Content
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 ;)
History
Date User Action Args
2020-06-07 11:03:35timmwagenersetrecipients: + timmwagener, asvetlov, yselivanov, aeros
2020-06-07 11:03:35timmwagenersetmessageid: <1591527815.25.0.526507516359.issue40894@roundup.psfhosted.org>
2020-06-07 11:03:35timmwagenerlinkissue40894 messages
2020-06-07 11:03:33timmwagenercreate