-
-
Notifications
You must be signed in to change notification settings - Fork 368
Add message with debugging info to Cancelled #3256
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
0b627ba
5b2294a
c7c46a4
af964cf
69c398b
357b797
1bc89b2
7def83f
8dbdde3
ebec334
87a8e07
36cf222
4944282
f37d993
67b1370
bcf0014
ada02cb
59d4eee
680306f
2fe4fca
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -73,27 +73,30 @@ class Cancelled(BaseException, metaclass=NoPublicConstructor): | |
|
||
""" | ||
|
||
source: Literal["deadline", "nursery", "explicit", "unknown", "KeyboardInterrupt"] | ||
# TODO: this should probably be a Task? | ||
source: Literal[ | ||
"KeyboardInterrupt", "deadline", "explicit", "nursery", "shutdown", "unknown" | ||
] | ||
# repr(Task), so as to avoid gc troubles from holding a reference | ||
source_task: str | None = None | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure Maybe a Nevermind, I didn't think this suggestion through. A weakref wouldn't work for the common case (a task cancelling a sibling task). I'm not convinced a strong ref here would be bad -- a Task doesn't store the exception or the cancellation reason so there's no reference cycle I think? But a string here is fine. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if a the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, I guess task -> parent/child nursery -> cancel scope -> cancel reason -> task is a loop, yeah. That's annoying. (Or maybe CoroutineType stores frames as a strongref? That too.) |
||
reason: str | None = None | ||
|
||
def __str__(self) -> str: | ||
def repr_if_not_none(lead: str, s: str | None, trail: str = "") -> str: | ||
def repr_if_not_none(lead: str, s: str | None, do_repr: bool = False) -> str: | ||
if s is None: | ||
return "" | ||
return lead + s + trail | ||
if do_repr: | ||
return lead + repr(s) | ||
return lead + s | ||
|
||
return ( | ||
f"cancelled due to {self.source}" | ||
+ repr_if_not_none(" with reason '", self.reason, "'") | ||
+ repr_if_not_none(" with reason ", self.reason, True) | ||
+ repr_if_not_none(" from task ", self.source_task) | ||
) | ||
|
||
def __reduce__(self) -> tuple[Callable[[], Cancelled], tuple[()]]: | ||
# the `__reduce__` tuple does not support kwargs, so we must use partial | ||
# for non-default args | ||
# or switch to allow posarg (?) | ||
return ( | ||
partial( | ||
Cancelled._create, | ||
|
@@ -111,7 +114,12 @@ def _create( | |
cls, | ||
*, | ||
source: Literal[ | ||
"deadline", "nursery", "explicit", "unknown", "KeyboardInterrupt" | ||
"KeyboardInterrupt", | ||
"deadline", | ||
"explicit", | ||
"nursery", | ||
"shutdown", | ||
"unknown", | ||
], | ||
source_task: str | None = None, | ||
reason: str | None = None, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -318,8 +318,14 @@ def expire(self, now: float) -> bool: | |
|
||
@attrs.define | ||
class CancelReason: | ||
# TODO: loren ipsum | ||
source: Literal["deadline", "nursery", "explicit", "KeyboardInterrupt"] | ||
"""Attached to a :class:`CancelScope` upon cancellation with details of the source of the | ||
cancellation, which is then used to construct the string in a :exc:`Cancelled`. | ||
Users can pass a ``reason`` str to :meth:`CancelScope.cancel` to set it. | ||
""" | ||
|
||
source: Literal[ | ||
"KeyboardInterrupt", "deadline", "explicit", "nursery", "shutdown", "unknown" | ||
] | ||
source_task: str | None = None | ||
reason: str | None = None | ||
|
||
|
@@ -580,8 +586,6 @@ class CancelScope: | |
_cancel_called: bool = attrs.field(default=False, init=False) | ||
cancelled_caught: bool = attrs.field(default=False, init=False) | ||
|
||
# necessary as cancel_status might be None | ||
# TODO: but maybe cancel_status doesn't need it? | ||
_cancel_reason: CancelReason | None = attrs.field( | ||
default=None, init=False, repr=True | ||
jakkdl marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
) | ||
|
@@ -1230,9 +1234,10 @@ def parent_task(self) -> Task: | |
"(`~trio.lowlevel.Task`): The Task that opened this nursery." | ||
return self._parent_task | ||
|
||
def _add_exc(self, exc: BaseException) -> None: | ||
def _add_exc(self, exc: BaseException, reason: CancelReason | None) -> None: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the only callers of this are internal, IMO it would be cleaner to have them set the reason inline. Also, to avoid multiple comments for similar things, why doesn't this unconditionally set There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
in case we get multiple sources of cancellation I don't want to override the first one. In other places it's more critical, but here I could see a scenario where:
so I'm pretty sure we need the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not relevant anymore There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I haven't looked at the code to see why this isn't relevant anymore, but I already typed up a response comment to this: I'm not entirely convinced avoiding this is a good thing: async def crasher():
await trio.sleep(2)
raise ValueError("...")
with trio.open_nursery() as nursery:
try:
nursery.start_soon(crasher)
try:
await trio.sleep(10) # app code
finally:
with trio.move_on_after(2, shield=True):
await trio.sleep(3) # cleanup code
except trio.Cancelled as c:
# what should c's cancel reason be
raise This might matter for instance in code for shutting down stuff on exceptions, moving on after 2 seconds. The cancel reason if the clean up code ran over its 2 seconds would presumably (?) be that the exceptions happened, not that the timeout happened. I think it would make more sense if the reason was instead about the timeout. (I haven't played with this PR yet so I'm not sure that's actually what will happen) Would it make sense to establish some sort of causal mechanism? I.e. a field on CancelReason that points to the old CancelReason. (I guess Cancelled could store another Cancelled? But that might be bad for cycles.) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah current behavior is that the crasher becomes the reason. Storing a chain of ... although the crash cancellation should be accessible somehow in the But storing a chain of reasons would be fairly straightforward and sounds like it might have some good use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought I found a repro where But going back to your example: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point: what's the cancel reason visible inside the move_on_after then? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm pretty sure it was There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice, yeah that behavior sounds nice. Returning to the earliest response you have, is that (nursery cancel -> raise a different exception in one of the tasks) the only case where things try to overwrite the cancellation reason? If so, I think it would be nicer to make nurseries not try to cancel if they are already cancelled (which would prevent the cancellation reason from being overwritten). I also see that changing the deadline can potentially overwrite. I don't see why that would try to cancel anything if things are already cancelled... I guess just code simplicity. I guess it makes sense to try to handle it in one place with a check on the cancellation reason, then. I just don't like it! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. haha. Yeah if we rewrote everything from scratch we might implement it differently. But I think there's a bunch of ways to re-cancel, including simply calling |
||
self._pending_excs.append(exc) | ||
# TODO: source/reason? | ||
if self.cancel_scope._cancel_reason is None: | ||
self.cancel_scope._cancel_reason = reason | ||
self.cancel_scope.cancel() | ||
|
||
def _check_nursery_closed(self) -> None: | ||
|
@@ -1249,11 +1254,14 @@ def _child_finished( | |
) -> None: | ||
self._children.remove(task) | ||
if isinstance(outcome, Error): | ||
if self.cancel_scope._cancel_reason is None: | ||
self.cancel_scope._cancel_reason = CancelReason( | ||
source="nursery", source_task=repr(task) | ||
) | ||
self._add_exc(outcome.error) | ||
self._add_exc( | ||
outcome.error, | ||
CancelReason( | ||
source="nursery", | ||
source_task=repr(task), | ||
A5rocks marked this conversation as resolved.
Show resolved
Hide resolved
|
||
reason=f"child task raised exception {outcome.error!r}", | ||
), | ||
) | ||
self._check_nursery_closed() | ||
|
||
async def _nested_child_finished( | ||
|
@@ -1263,7 +1271,14 @@ async def _nested_child_finished( | |
# Returns ExceptionGroup instance (or any exception if the nursery is in loose mode | ||
# and there is just one contained exception) if there are pending exceptions | ||
if nested_child_exc is not None: | ||
self._add_exc(nested_child_exc) | ||
self._add_exc( | ||
nested_child_exc, | ||
reason=CancelReason( | ||
source="nursery", | ||
source_task=repr(self._parent_task), | ||
reason=f"Code block inside nursery contextmanager raised exception {nested_child_exc!r}", | ||
), | ||
) | ||
self._nested_child_running = False | ||
self._check_nursery_closed() | ||
|
||
|
@@ -1274,7 +1289,13 @@ async def _nested_child_finished( | |
def aborted(raise_cancel: _core.RaiseCancelT) -> Abort: | ||
exn = capture(raise_cancel).error | ||
if not isinstance(exn, Cancelled): | ||
self._add_exc(exn) | ||
self._add_exc( | ||
exn, | ||
CancelReason( | ||
source="KeyboardInterrupt", | ||
source_task=repr(self._parent_task), | ||
), | ||
) | ||
# see test_cancel_scope_exit_doesnt_create_cyclic_garbage | ||
del exn # prevent cyclic garbage creation | ||
return Abort.FAILED | ||
|
@@ -1288,7 +1309,8 @@ def aborted(raise_cancel: _core.RaiseCancelT) -> Abort: | |
try: | ||
await cancel_shielded_checkpoint() | ||
except BaseException as exc: | ||
self._add_exc(exc) | ||
# there's no children to cancel, so don't need to supply cancel reason | ||
self._add_exc(exc, reason=None) | ||
|
||
popped = self._parent_task._child_nurseries.pop() | ||
assert popped is self | ||
|
@@ -2134,6 +2156,12 @@ async def init( | |
|
||
# Main task is done; start shutting down system tasks | ||
# TODO: source/reason? | ||
self.system_nursery.cancel_scope._cancel_reason = CancelReason( | ||
source="shutdown", | ||
reason="main task done, shutting down system tasks", | ||
source_task=repr(self.init_task), | ||
) | ||
|
||
self.system_nursery.cancel_scope.cancel() | ||
|
||
# System nursery is closed; finalize remaining async generators | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One missing test: how about a task that is stated via There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh good catch, currently the reason becomes "Code block inside nursery contextmanager raised exception [...]" - which is quite misleading There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hm, I'm not sure how to fix that other than to just make the message more generic. There's not really any way of distinguishing the two cases, and the user might handle the exception that was generated in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we catch an exception from the helper nursery and then if so, pre-emptively cancel with a better reason? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The only sort-of-reasonable way I can see of doing it is to save the exception that is raised from But even if the reason might be slightly misleading, I think the reasonable next step in debugging is for the developer to look up backtraces to find out what the exception is that killed the CM block, and they will then quickly find out what happened. So thinking about it a bit more I'm not sure this is an issue in practice There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd vote to merge, and then revisit if anyone reports that this is an issue in practice. |
Uh oh!
There was an error while loading. Please reload this page.