-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
WIP: implement cancellation and raw handles #2474
Conversation
7f9adb6
to
4ef9002
Compare
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.
Hey, sorry for the delay. I am finally getting to this and it is definitely something that I think we should move forward with. The structured concurrency strategy has been put on hold indefinitely as we don't have a way forward with it.
I suggested splitting out the RawJoinHandle
part. I would suggest starting a discussion issue for that.
As far as I can tell, the actual cancellation bit is not too involved as it can leverage shutdown()
. I would probably recommend naming it something a bit more forceful like abort()
?
What do you think?
tokio/src/runtime/task/join.rs
Outdated
/// } | ||
/// } | ||
/// ``` | ||
pub fn into_raw_handle(self) -> RawJoinHandle { |
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 not necessarily against this, but could we decouple exposing RawJoinHandle
as a public API from this PR? I would like to explore alternate solutions like providing a custom collection type.
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.
Rebased and renamed to abort
for now.
How do you want me to hide this from the public API?
Completely remove it, make private with #[allow(unused)]
, or #[doc(hidden)]
?
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.
Completely remove it in this PR. I would like to get abort landed. I don’t think a raw handle is the path forward. Lets discuss the use cases that motivated you that direction so we can figure out an option.
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.
Removed, thanks!
9e42b11
to
f9c6d02
Compare
f9c6d02
to
7276d47
Compare
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.
Looks great 👍 thanks! Sorry for the delay.
I have a question that I feel the documentation of this method should answer. Will the task be cancelled (read dropped) the next time it is polled? The next time the joinhandle get's polled? Does the joinhandle need to be awaited for it to be dropped or just the task polled? Ok, sorry, that's 3 questions. You get the issue I think. |
@najamelan The task is de-scheduled and dropped in the background, regardless if you poll it or not. The |
Thanks for clarifying. What happens if the future is currently running? I know in futures (Abortable/RemoteHandle), the canceling happens in the poll method, so |
Just for clarity: a task associated with a future is either being polled (i.e. running), or it's parked. If a task is cancelled in the middle of a poll the scheduler will notice this when the poll returns, and regardless of whether it returns |
This is a WIP PR for implementing a simple form of cancellation, as mentioned here. Since work on structured concurrency is underway this might not be necessary, but I'm setting this up so that others can have a look and provide input.
While related to structured concurrency it does not replace it. It is only a low-level primitive necessary to perform some best-effort graceful shutdown (#1819) over spawned tasks.
Note that this does not allow for graceful termination of tasks spawned with
spawn_blocking
that are actively blocking, which includes mostFile
operations.Warning: Naming is provisional, and certain assumptions around task shutdown might not be correct since I'm still not super familiar with the task system.
Motivation
Support for some form of cancellation is necessary to support graceful termination . This doesn't replace structured concurrency, but should be considered more of an incremental step towards allowing application and library authors to support graceful termination of background tasks without adopting a different API.
Solution
Hooks into the existing task system, and extends completion polling with a variant which doesn't export the output value to support raw join handles. This allows us to cancel tasks (which are currently not being polled), and
.await
their termination. Hopefully providing theirOutput
or a simple completion signal on a best-effort basis.