Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Expand cancellation usability from native trio threads #2392
Expand cancellation usability from native trio threads #2392
Changes from 22 commits
d9d02db
0c0ff10
5f86fb2
5f75c86
7ccf0f7
6d3b2d6
2d911ea
bc6c82a
644b913
b87456f
f6b27b2
2b088cc
832da41
5a10e9b
a09aa84
787d286
881180f
c0e11d4
1930cae
b02dfe9
1bb9b79
689d45c
c1990d4
d4d10bb
daed7bb
2be054a
a667a52
d6df308
9f4e79e
0e18c93
eab30c4
2f79f15
96e45c5
ab092b0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, this suggests that
from_thread.run_sync
does raise Cancelled if the thread was abandoned. Where is that coming from? It can't be from the cancel scope, because the cancel scope doesn't exist anymore (in the general case). If it's a general "this thread was abandoned" marker, then that's mildly backcompat-breaking and I think I disprefer it regardless - "sync functions have no checkpoints and therefore can't be the cause of a Cancelled" is a nice and easy rule to remember and works better if we don't have corner-case exceptions to it.I'm not sure how async
from_thread.run
from an abandoned thread should work. Options I see are:I would prefer one of the first two, since the third is unlike anything done elsewhere in Trio (Cancelled is raised at checkpoints, entry to an async function is not a checkpoint) and makes it hard to hand off resources safely (if you 'own' a resource and you pass it to a task that begins
[async] with resource:
, then that's normally safe but wouldn't be with the new semantics because Cancelled could be raised before the recipient enters the context manager).Regardless of what we choose, we need to document it carefully, because it's a subtle consideration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I could on board for "run in cancelled system task" but documenting the nuances of "if you abandon your thread, your async functions are going to run in a weird different task" seems really hard.
What do you think of re-using
RunFinishedError
or making aTaskFinishedError
to raise immediately? This would at least not trample on the semantics ofCancelled
, but it wouldn't help with the resource handoff issue (on the other hand, users must already handleRunFinishedError
in the present case).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I implemented the semantics I think you expected, and there were enough degrees of freedom in the test suite that it didn't need much adjustment. That makes me think that we should think of a way to (a) document the surprising abandon-to-system-task semantics and (b) anchor them in the test suite. I can't yet think of which behaviors really should be represented that way though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, on further consideration I agree my previous proposal is fairly wonky and we can probably do better. I think my preference would actually be to have
cancellable=True
threads use the old semantics (whether they've actually been cancelled yet or not): don't pass cancellation, don't runfrom_thread.run
reentrant calls in theto_thread.run_sync
task, just use a system task (that is not cancelled) like we did before this PR. Rationale:cancellable=True
thread explicitly might outlive the original task, any attempt it makes to call back into Trio is probably for something unrelated to the original task's context, so it's strange for that attempt to reflect the cancellation status of the original task.cancellable=False
threads being an "extension of the current task", vscancellable=True
being "fire-and-forget + retrieve the result if you're still around when it becomes available". It just feels wrong to propagate cancellation in the latter case.This discussion is underscoring for me that
cancellable=True
is a bad name;cancellable=False
threads are arguably more cancellable! My first thought for a replacement isdetachable
but maybe you have a better idea. Doesn't need to go in this PR but maybe should arrive in the same release to reduce confusion.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was going to suggest a new, overriding kwarg
abandon_on_cancel
. That tells you what happens and when, and my multiprocessing library could usekill_on_cancel
instead.cancellable
would then hang around indefinitely, deprecated but with unchanged semantics.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm equally happy with
abandon_on_cancel
as a name.