Ben Buchwald <spar...@gmail.com> 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 <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
