Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: added spawn_aborting #6224

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

feat: added spawn_aborting #6224

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
794649f
feat: added spawn_aborting
Dec 18, 2023
bb34668
Merge branch 'master' into master
Darksonn Dec 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions tokio-util/src/task/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,3 +13,6 @@ pub use join_map::{JoinMap, JoinMapKeys};

pub mod task_tracker;
pub use task_tracker::TaskTracker;

pub mod spawn_aborting;
pub use spawn_aborting::spawn_aborting;
38 changes: 38 additions & 0 deletions tokio-util/src/task/spawn_aborting.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
use std::ops::{Deref, DerefMut};

use tokio::task::JoinHandle;

use futures_core::Future;
Copy link
Contributor

@Darksonn Darksonn Dec 30, 2023 •
edited

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 is a wrapper type around JoinHandle that allows it to be dropped.
Copy link
Member

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:

Suggested change
/// 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

#[derive(Debug)]
pub struct DropHandle<T>(JoinHandle<T>);
Comment on lines +7 to +9
Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Member

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> Drop for DropHandle<T> {
fn drop(&mut self) {
self.0.abort();
}
}

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
}
}
Comment on lines +17 to +29
Copy link
Contributor

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.


/// 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))
}
Comment on lines +31 to +38
Copy link
Contributor

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?

Copy link
Contributor

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.

Copy link

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.

Copy link
Member

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 JoinHandles 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))
    }
}