Message370886
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 ;) |
|
Date |
User |
Action |
Args |
2020-06-07 11:03:35 | timmwagener | set | recipients:
+ timmwagener, asvetlov, yselivanov, aeros |
2020-06-07 11:03:35 | timmwagener | set | messageid: <1591527815.25.0.526507516359.issue40894@roundup.psfhosted.org> |
2020-06-07 11:03:35 | timmwagener | link | issue40894 messages |
2020-06-07 11:03:33 | timmwagener | create | |
|