-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
feat: added spawn_aborting #6224
Conversation
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.
Hi. Sorry about the delay in reviewing during the christmas holidays.
|
||
use tokio::task::JoinHandle; | ||
|
||
use futures_core::Future; |
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.
Please import Future
from the standard library instead.
/// This function spawns a task that aborts on drop instead of lingering. | ||
pub fn spawn_aborting<F>(future: F) -> DropHandle<F::Output> | ||
where | ||
F: Future + Send + 'static, | ||
F::Output: Send + 'static, | ||
{ | ||
DropHandle(tokio::spawn(future)) | ||
} |
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 don't like this name. It sounds like something that would immediately make something abort.
How about spawn_with_drop_handle
or spawn_with_abort_handle
?
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.
Another possibility is to make this a constructor method. Then you would type DropHandle::spawn
.
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.
For API naming, how about using spawn_abortable
which aligns with Abortable in future crate.
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.
Hmm, I don't love spawn_abortable
, because in Tokio, all spawned tasks are already abortable using JoinHandle::abort()
. Naming the function spawn_abortable
kind of implies that all other tasks cannot be aborted.
My personal preference would be to make the spawn function a function on the DropHandle
type. IMO, we also ought to have a DropHandle::new()
constructor that takes a JoinHandle
, so that the abort-on-drop behavior can be added to JoinHandle
s returned by tokio::task::spawn_local
or tokio::task::spawn_blocking
.
I'd probably recommend an interface that looks sort of like this:
impl<T> DropHandle<T> {
pub fn new<T>(task: JoinHandle<T>) -> Self{
Self(task)
}
pub fn spawn(future: impl Future<Output = T> + Send + 'static) -> DropHandle<T> {
Self(tokio::spwan(future))
}
}
/// This is a wrapper type around JoinHandle that allows it to be dropped. | ||
#[derive(Debug)] | ||
pub struct DropHandle<T>(JoinHandle<T>); |
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.
This name is okay, but I think there is precedent for the name AbortHandle
elsewhere, so we might want to prefer that name.
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.
Actually, we have a type called AbortHandle
in Tokio, and that does something else ... so that name doesn't work.
But another option is AbortOnDropHandle
. I'll let you pick.
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.
AbortOnDropHandle
is uncomfortably verbose, but I think it's the most descriptive name...the name should make it clear what this type actually does.
impl<T> Deref for DropHandle<T> { | ||
type Target = JoinHandle<T>; | ||
|
||
fn deref(&self) -> &Self::Target { | ||
&self.0 | ||
} | ||
} | ||
|
||
impl<T> DerefMut for DropHandle<T> { | ||
fn deref_mut(&mut self) -> &mut Self::Target { | ||
&mut self.0 | ||
} | ||
} |
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.
Instead of this, could you add all of the wrapper methods here? You can implement AsRef<JoinHandle>
if you like.
It would probably also be nice with a Future
implementation so that you can .await
a DropHandle
.
It seems like there's a compilation failure in CI:
However, in this case, I think the answer might just be that the module should not be public. |
Should this be must_use? Something like |
/// This function spawns a task that aborts on drop instead of lingering. | ||
pub fn spawn_aborting<F>(future: F) -> DropHandle<F::Output> | ||
where | ||
F: Future + Send + 'static, | ||
F::Output: Send + 'static, | ||
{ | ||
DropHandle(tokio::spawn(future)) | ||
} |
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.
Hmm, I don't love spawn_abortable
, because in Tokio, all spawned tasks are already abortable using JoinHandle::abort()
. Naming the function spawn_abortable
kind of implies that all other tasks cannot be aborted.
My personal preference would be to make the spawn function a function on the DropHandle
type. IMO, we also ought to have a DropHandle::new()
constructor that takes a JoinHandle
, so that the abort-on-drop behavior can be added to JoinHandle
s returned by tokio::task::spawn_local
or tokio::task::spawn_blocking
.
I'd probably recommend an interface that looks sort of like this:
impl<T> DropHandle<T> {
pub fn new<T>(task: JoinHandle<T>) -> Self{
Self(task)
}
pub fn spawn(future: impl Future<Output = T> + Send + 'static) -> DropHandle<T> {
Self(tokio::spwan(future))
}
}
/// This is a wrapper type around JoinHandle that allows it to be dropped. | ||
#[derive(Debug)] | ||
pub struct DropHandle<T>(JoinHandle<T>); |
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.
AbortOnDropHandle
is uncomfortably verbose, but I think it's the most descriptive name...the name should make it clear what this type actually does.
|
||
use futures_core::Future; | ||
|
||
/// This is a wrapper type around JoinHandle that allows it to be dropped. |
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.
A JoinHandle
can already be dropped, just like all other Rust values. So, this documentation, while technically correct, isn't really explaining what this type is actually for. How about something more like this:
/// This is a wrapper type around JoinHandle that allows it to be dropped. | |
/// A wrapper around a [`tokio::task::JoinHandle`] which [aborts] the task | |
/// automatically when the handle is dropped. | |
/// | |
/// [aborts]: tokio::task::JoinHandle::abort |
wraps a `JoinHandle`, aborting the task on drop Refs: tokio-rs#6224
wraps a `JoinHandle`, aborting the task on drop Refs: tokio-rs#6224 Fixes: tokio-rs#6160
wraps a `JoinHandle`, aborting the task on drop Refs: tokio-rs#6224 Fixes: tokio-rs#6160
wraps a `JoinHandle`, aborting the task on drop Refs: tokio-rs#6224 Fixes: tokio-rs#6160
wraps a `JoinHandle`, aborting the task on drop Refs: tokio-rs#6224 Fixes: tokio-rs#6160
wraps a `JoinHandle`, aborting the task on drop Co-authored-by: Alice Ryhl <aliceryhl@google.com> Refs: tokio-rs#6224 Fixes: tokio-rs#6160
wraps a `JoinHandle`, aborting the task on drop Co-authored-by: Alice Ryhl <aliceryhl@google.com> Refs: tokio-rs#6224 Fixes: tokio-rs#6160
This feature was added in #6786. |
Motivation
#6160
Solution
A wrapper type around the JoinHandle