Timm Wagener <wagenert...@gmail.com> 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 <rep...@bugs.python.org> <https://bugs.python.org/issue40894> _______________________________________ _______________________________________________ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
