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
<https://bugs.python.org/issue40894>
___
___
Python-bugs-list mailing list
Unsubscribe:
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
