bpo-46771: Remove two controversial lines from Task.cancel() - #31623
Conversation
Sorry, something went wrong.
Also from the _asyncio C accelerator module, and adjust one test that the change caused to fail. For more discussion see the discussion starting here: #31394 (comment) (Basically, @asvetlov proposed to return False from cancel() when there is already a pending cancellation, and I went along, even though it wasn't necessary for the task group implementation, and @agronholm has come up with a counterexample that fails because of this change. So now I'm changing it back to the old semantics (but still bumping the counter) until we can have a proper discussion about this.)
|
This seems like a step in the right direction, but further changes will be necessary. |
Sorry, something went wrong.
Could you be more specific? What use case are you thinking of? |
Sorry, something went wrong.
asvetlov
left a comment
There was a problem hiding this comment.
I'm fine with this.
.uncancel() makes the change not necessary.
Deprecation of cancellation message clears my other objections.
Sorry, I'm not active this week. Hard events in my life :(
Sorry, something went wrong.
Unless more changes are made, old-style cancel scope-like context managers will distort the cancel counter. It should be reset to 0 when the task is made cancellable again (when the CancelledError has been delivered to the task). This should improve the interaction of counter-aware context managers and non-aware ones by letting the aware ones function correctly in the case of a re-cancellation when the aware context manager is the outer one and the non-aware one is the inner one. |
Sorry, something went wrong.
|
Ideally I would have liked the CancelledError to carry this information. That would make it easier to write code compatible with 3.9 - 3.10 since those versions will have to use the cancel message trick. |
Sorry, something went wrong.
|
@agronholm Would you mind submitting a (draft) PR? I personally think that if we have a solution that will work great in the future, making the existing cancel scope implementations have to do a version check would be a small price to pay. |
Sorry, something went wrong.
|
Could you also tag me in that PR? Trying to wrap my head around the issue. |
Sorry, something went wrong.
|
I'm currently on a work trip but will try work on that in the next few days. |
Sorry, something went wrong.
Also from the _asyncio C accelerator module,
and adjust one test that the change caused to fail.
For more discussion see the discussion starting here:
#31394 (comment)
(Basically, @asvetlov proposed to return False from cancel()
when there is already a pending cancellation, and I went along,
even though it wasn't necessary for the task group implementation,
and @agronholm has come up with a counterexample that fails
because of this change. So now I'm changing it back to the old
semantics (but still bumping the counter) until we can have a
proper discussion about this.)
https://bugs.python.org/issue46771