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.

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

JoinHandle variant that abort on drop. #6160

Closed
HyeonuPark opened this issue Nov 19, 2023 · 9 comments · Fixed by #6786
Closed

JoinHandle variant that abort on drop. #6160

HyeonuPark opened this issue Nov 19, 2023 · 9 comments · Fixed by #6786
Labels
A-tokio Area: The main tokio crate C-feature-request Category: A feature request. M-task Module: tokio/task

Comments

@HyeonuPark
Copy link
Contributor

HyeonuPark commented Nov 19, 2023

Is your feature request related to a problem? Please describe.
Prior arts: #1830 #2560

After debugging few task leaks I started to make my own JoinHandle<T> wrapper that abort on drop and using it. It's quite useful when you understand the flow and the cancelable nature of async tasks.

Describe the solution you'd like
Add const generic bool parameter ABORT_ON_DROP to the JoinHandle<T> type with default value false. JoinHandle<T, true> will .abort() itself on its impl Drop. Add fn abort_on_drop(self) -> JoinHandle<T, true> method on JoinHandle<T, false> to construct this type.

Describe alternatives you've considered
JoinSet<T> is nice. I'm using it when appropriate. But it's far suboptimal for fixed number of heterogeneous tasks.

Like my local solution, this functionality can also be provided by separate newtype over JoinHandle<T>. This newtype should also impl every methods and traits the JoinHandle<T> have. But the hardest part is naming. Honestly I can't imagine a single good name for it.

Instead of having type level flag the JoinHandle<T> can actually holds additional bool flag at runtime for it. But I failed to find a good place to store it within the JoinHandle<T>'s structure.

Additional context

@HyeonuPark HyeonuPark added A-tokio Area: The main tokio crate C-feature-request Category: A feature request. labels Nov 19, 2023
@Darksonn Darksonn added the M-task Module: tokio/task label Nov 21, 2023
@Darksonn
Copy link
Contributor

Although the idea seems somewhat reasonable, I'm not convinced by the generic boolean. I would rather have a separate type.

@HyeonuPark
Copy link
Contributor Author

Thanks for the feedback. Remainders would be the type placement and the naming as implementation would be trivial. I think under tokio::task would be the place but tokio_util::task sill make sense if you prefer. For naming, here's a list of names squeezed from my brain. I'm not particularly convinced by any of them, please tell me one if you have better name. If I need to pick one now, I'll choose TaskGuard.

  • AbortOnDropJoinHandle
  • TaskGuard
  • StructuredJoinHandle
  • ScopedJoinHandle

@hawkw
Copy link
Member

hawkw commented Nov 21, 2023

I think that we'd probably prefer to have something like this in tokio_util, rather than the core tokio crate, just because it's possible to implement it entirely outside of tokio, and we generally prefer to avoid adding APIs to the main crate that don't need to be in the main crate. This is because we can't easily make breaking changes to APIs in tokio (as it's 1.0), but we can easily make breaking changes to APIs in tokio_util.

Of the naming suggestions, I think TaskGuard is the nicest, although I also feel like it's potentially a good idea to keep the phrase "JoinHandle" in the name, to make the connection between the new type and the core JoinHandle type obvious...

@dev-ardi
Copy link

What about DropHandle or GuardHandle?

@kornelski
Copy link
Contributor

I need this functionality too. Could this be as simple as .abort_on_drop() method on the existing JoinHandle?

let _guard = spawn(async {}).abort_on_drop();

It could be either setting some boolean on the existing JoinHandle, or returning it wrapped in a newtype.

@Darksonn
Copy link
Contributor

I still prefer a newtype. Using the existing type makes JoinHandle larger.

@nazar-pc
Copy link

I maintained AsyncJoinOnDrop for some time now that achieves something similar: https://github.com/subspace/subspace/blob/7b62292b63aaaac3a1b8794f3c39eba5c1314a86/crates/subspace-farmer/src/utils.rs#L26-L73

The additional requirement I had that is not mentioned above is when the task is actually physically dropped. For me it was important that task is actually dropped by the time Drop::drop is over, so I had to add a check and use block_in_place to wait for completion. Whether to abort the task or wait until completion was made an option. Maybe something to take into consideration (and yes, I am looking forward to some kind of solution with async drop).

@Darksonn
Copy link
Contributor

Tokio will never use block_in_place for something like this. That function has way too many caveats (e.g., doesn't work in current thread runtime, incompatible with tokio::select!, tokio::join!, FuturesUnordered, etc.).

@RangerMauve
Copy link

This would be great to have for tests where I spawn some threads and am using .expect or .unwrap in assertions.

barafael added a commit to barafael/tokio that referenced this issue Aug 17, 2024
wraps a `JoinHandle`, aborting the task on drop
Refs: tokio-rs#6224
Fixes: tokio-rs#6160
barafael added a commit to barafael/tokio that referenced this issue Aug 17, 2024
wraps a `JoinHandle`, aborting the task on drop
Refs: tokio-rs#6224
Fixes: tokio-rs#6160
barafael added a commit to barafael/tokio that referenced this issue Aug 19, 2024
wraps a `JoinHandle`, aborting the task on drop
Refs: tokio-rs#6224
Fixes: tokio-rs#6160
barafael added a commit to barafael/tokio that referenced this issue Aug 19, 2024
wraps a `JoinHandle`, aborting the task on drop
Refs: tokio-rs#6224
Fixes: tokio-rs#6160
barafael added a commit to barafael/tokio that referenced this issue Aug 19, 2024
wraps a `JoinHandle`, aborting the task on drop

Co-authored-by: Alice Ryhl <aliceryhl@google.com>
Refs: tokio-rs#6224
Fixes: tokio-rs#6160
barafael added a commit to barafael/tokio that referenced this issue Aug 19, 2024
wraps a `JoinHandle`, aborting the task on drop

Co-authored-by: Alice Ryhl <aliceryhl@google.com>
Refs: tokio-rs#6224
Fixes: tokio-rs#6160
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio Area: The main tokio crate C-feature-request Category: A feature request. M-task Module: tokio/task
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants