Fixed cancellation propagation when task group host is in a shielded scope #648
Conversation
src/anyio/_backends/_asyncio.py
Outdated
| def _deliver_cancellation_to_parent(self) -> None: | ||
| """Start cancellation effort in the farthest directly cancelled parent scope""" | ||
| def _restart_cancellation_in_parent(self) -> None: | ||
| """Start cancellation effort in the closest directly cancelled parent scope""" | ||
| scope = self._parent_scope | ||
| scope_to_cancel: CancelScope | None = None | ||
| while scope is not None: | ||
| if scope._cancel_called and scope._cancel_handle is None: | ||
| scope_to_cancel = scope | ||
| if scope._cancel_called: | ||
| if scope._cancel_handle is None: | ||
| scope._deliver_cancellation(scope) | ||
|
|
||
| break | ||
|
|
||
| # No point in looking beyond any shielded scope | ||
| if scope._shield: | ||
| break | ||
|
|
||
| scope = scope._parent_scope | ||
|
|
||
| if scope_to_cancel is not None: | ||
| scope_to_cancel._deliver_cancellation() |
There was a problem hiding this comment.
would the following simplification be correct?:
def _restart_cancellation_in_parent(self) -> None:
"""Restart cancellation effort in the parent scope if it was directly cancelled"""
scope = self._parent_scope
if scope is not None and scope._cancel_called and scope._cancel_handle is None:
scope._deliver_cancellation(scope)it passes all tests. i think that _deliver_cancellation_to_parent previously needed to walk up beyond self._parent_scope because self._parent_scope.__exit__ was not guaranteed to call self._parent_scope._deliver_cancellation_to_parent. but with the call in __exit__ unconditional now, i think walking up beyond self._parent_scope is not needed.
There was a problem hiding this comment.
Yeah, it looks good. I've pushed this change now.
There was a problem hiding this comment.
oops—i've just realized that this simplification is actually incorrect. suggested changes: a7284b7...bfef03b
There was a problem hiding this comment.
It's embarrassing that I didn't realize that myself. Reverted.
Co-authored-by: Ganden Schaffner <[email protected]>
|
All ready for merging then? |
This reverts commit a7284b7.
Co-authored-by: Ganden Schaffner <[email protected]>
|
might be good to add 0214c34, as that bug almost slipped by. |
Done. |
|
No worries! Would you mind approving the PR if it looks good? |
|
Hm, that didn't do the trick. It doesn't look like you're on the collaborator list. Would you like to be? |
|
sure! |
|
invite accepted; should be merge-able now. |
|
Thanks for the review! |
This makes the following changes:
Fixes #642.