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

Consider making JoinHandle cancel on drop #1830

Closed
seanmonstar opened this issue Nov 27, 2019 · 26 comments
Closed

Consider making JoinHandle cancel on drop #1830

seanmonstar opened this issue Nov 27, 2019 · 26 comments
Labels
A-tokio Area: The main tokio crate C-proposal Category: a proposal and request for comments M-task Module: tokio/task
Milestone

Comments

@seanmonstar
Copy link
Member

In Tokio 0.2, tokio::task::spawn now returns a JoinHandle which is a Future of when the task completes. It currently behaves the same as std::thread::JoinHandle, which will "detach" it automatically if ignored.

I want to propose that we change this behavior such that dropping a JoinHandle cancels the associated task.

Motivation

  • The main reason I can see for why std::thread::JoinHandle doesn't cancel the associated thread when dropped is because there's no real way to cancel an OS thread.
  • It's trivial to cancel a Task.
  • The 0.2 default makes it very easy to "leak" tasks. In order to propagate cancellation, the user needs to know about that and include an extra oneshot or similar. It can also be surprising that even if a user did try to hold onto the JoinHandle to prevent a leak, if the wrapping future is dropped, the spawned task will still "leak".

Change

  • #[must_use = "Dropping a JoinHandle will cancel the spawned task."]
    pub struct JoinHandle<T> { .. }
  • Add JoinHandle::daemonize() (or detach or background) so that "leaking" some task is an explicit action.
@seanmonstar seanmonstar added this to the v0.3 milestone Nov 27, 2019
@seanmonstar seanmonstar added the C-proposal Category: a proposal and request for comments label Nov 27, 2019
@hawkw
Copy link
Member

hawkw commented Nov 27, 2019

As an alternative, we could add a JoinHandle::cancel_on_drop to make the cancellation behavior opt-in, rather than opt-out. This would mean the API is still essentially a superset of std — the differences would be additive in nature.

I'm not sure if this is a better approach to the proposed change, but it's worth considering. It's worth noting that since it's not a breaking change, we could get an opt-in task cancellation change in before 0.3. That way, users can start trying it out sooner, and if it proves to be the right thing, we could make it the default in 0.3.

Personal opinions to follow:


IMO, opting in to "leaking" rather than opting out is a better design in isolation. However, it's strange to have an API that's almost exactly identical to std but differs in one key way. If the type was called something other than JoinHandle, I might feel differently.

  • Add JoinHandle::daemonize() (or detach or background) so that "leaking" some task is an explicit action.

My vote is for background.

@vorot93
Copy link
Member

vorot93 commented Dec 10, 2019

Cancel on drop would greatly simplify cancellation of dependent tasks and effectively obsolete hoops like future::Abortable. I would be very happy to see this in Tokio.

On top of that, I would support making it the default behaviour. While it is not really feasible to cancel the running thread, tasks are not subject to this limitation. Therefore we have an option of aligning this part of API with idiomatic Rust by defaulting to managing said task via a handle just like any other resource. It should have higher priority than blindly mimicking std.

@bikeshedder
Copy link
Contributor

I feel like the entire idea of tokio::spawn is to spawn a new task in the background. I think of spawn like fork. Start a separate process/task and return its handle which can be used for joining.

If would much prefer it the other way around. Add a JoinHandle::join() function which converts it into a struct that stops the task when dropped:

tokio::spawn(fut).join().await;

Also as @carllerche just wrote in the Discord chat: It would be very confusing if the following code would become a noop because the handle is dropped immediately.

tokio.spawn(fut);

@hawkw
Copy link
Member

hawkw commented Dec 10, 2019

Also as @carllerche just wrote in the Discord chat: It would be very confusing if the following code would become a noop because the handle is dropped immediately.

tokio.spawn(fut);

FWIW, I think this issue in particular could be solved just by putting a #[must_use] on JoinHandle.

@carllerche
Copy link
Member

@hawkw it doesn't, because during early dev, it's usual to have a wall of warnings while "getting something working"... the warning will be lost.

@carllerche
Copy link
Member

I also think that "cancel on drop" is only a partial solution. A cancel on drop will be a forceful cancellation which will prevent the task from being able to do cleanup.

ATM, I am much more keen on a holistic approach to solving the problem: #1879

@hawkw
Copy link
Member

hawkw commented Dec 10, 2019

@hawkw it doesn't, because during early dev, it's usual to have a wall of warnings while "getting something working"... the warning will be lost.

Sure, but if the spawned task is immediately cancelled, then the code won't work, and the warning would make it much clearer why it doesn't work.

I think if we have a tokio::spawn_background fn (or similar) that doesn't return a JoinHandle, and if the must_use warning clearly stated "if you are ignoring the result of this task, use spawn_background instead" or something, this wouldn't be a serious issue.

@najamelan
Copy link
Contributor

I find it unfortunate to solve these issues at the framework level. The need for a JoinHandle type that can "detach", "drop" and return a value is there for all async rust code, and it would allow libraries to use it without locking their clients into a specific framework.

For now the futures library has RemoteHandle and Abortable. Unfortunately the API's are poor:

  • You cannot really rely on RemoteHandle to drop the future unless if it is being polled frequently, because it will only drop the remote future the next time it get's polled (which might be never).
  • remote_handle does not require Send on the future but it does so on the return type.
  • remote_handle will catch_unwind, which seems like a magic decision that should belong to client code?
  • Abortable does not return a value.

What if the futures library would have a satisfying JoinHandle that everybody could use? Would it be possible to achieve a consensus around it's requirements/implementation?

ATM, I am much more keen on a holistic approach to solving the problem: #1879

I might have to think and experiment a bit more, but I think (forcibly) cancellable tasks are a fundamental building block to making structured concurrency happen. Since you're going to join every task, you can't afford a random task blocking the whole application.

@najamelan
Copy link
Contributor

It turns out I was mistaking about RemoteHandle not waking up on drop. The issue I had is still somewhat mysterious, but it's not in RemoteHandle.

Which makes me wonder, what is the motivation for not using it but rolling a custom JoinHandle in each framework?

@udoprog
Copy link
Contributor

udoprog commented Dec 29, 2019

It turns out I was mistaking about RemoteHandle not waking up on drop. The issue I had is still somewhat mysterious, but it's not in RemoteHandle.

Which makes me wonder, what is the motivation for not using it but rolling a custom JoinHandle in each framework?

RemoteHandle enforces the use of a oneshot channel to send the result, while an integrated handle (e.g. JoinHandle) can interact directly with the task system. Reducing the amount of indirection and allowing it to be more efficient.

@cynecx
Copy link
Contributor

cynecx commented Mar 2, 2020

Just wondering, is it possible to cancel tasks right now with the current api? If not what's the "best" current workaround for cancelling tasks?

@najamelan
Copy link
Contributor

@cynecx you can use futures::Abortable. Also I just released async_executors crate which makes a framework agnostic JoinHandle which uses futures::Abortable to create consistent API and which wraps the native joinhandle to avoid the overhead from RemoteHandle. It can be used with tokio.

@LucioFranco
Copy link
Member

You can also just select the task against a oneshot for example. I think making joinhandle cancel on drop can be surprising.

@peku33
Copy link

peku33 commented Apr 5, 2020

Wouldn't cancel-on-drop make it easier to implement spawn() for non-static futures?
spawning future wrapped in abortable does not guarantee that future is no longer polled when abort is called. As far as I understand abort stops on next poll, but if the future is still running (for example busy on cpu intensive task on another thread) there is no guarantee it is stopped or dropped when abort() completes. Abortable future must still be awaited before.

@najamelan
Copy link
Contributor

@peku33 yes, a currently running poll will complete before the future can be aborted. But that being said, how would you stop a sequence of instructions running in the cpu if that sequence provides no means of being interrupted?

@Darksonn
Copy link
Contributor

Darksonn commented Apr 5, 2020

@peku33 ultimately cancel-on-drop does not help with non-static futures because of mem::forget.

@carllerche
Copy link
Member

We discussed this in slack and decided not to move forward with this.

There was no consensus either way, but most were either not strong advocates, on the fence, or against. There also is not enough time to experiment with this before 1.0. Not canceling on drop matches std behavior around thread::spawn. Additionally, cancelling on drop would resuilt in most current examples to just "not work". tokio::spawn(my_task); would be a no-op. This could be mitigated by a lint, but it is very common for beginners to miss lints.

It is also worth noting that a forceful cancellation is almost never the correct behavior. One needs to perform a graceful cancellation where a shutdown signal is sent followed by joining on the handle. The primary argument for cancel on drop is that it is "easier" to catch a no-op bug than leaking tasks.

Tokio provides the necessary infrastructure to implement this behavior externally. This can be done by wrapping the JoinHandle with a drop implementation that calls abort() on the inner handle. Cancel on drop behavior can be experimented with in an external crate.

@szagi3891
Copy link

@carllerche - But this structure doesn't have an "abort" method.
https://docs.rs/tokio/0.2.22/tokio/task/struct.JoinHandle.html

@Darksonn
Copy link
Contributor

@szagi3891 It was added by #2474, which was merged 14 days ago.

@szagi3891
Copy link

Thank you for the information, I'll be happy to use it :) When is the new version of "tokyo" coming out?

And is it possible to read the number of "tasks"? This could be useful for checking that a task is not leaking.

@carllerche
Copy link
Member

Task spawning is instrumented w/ tracing, so it should be possible to get that info. cc @hawkw

@depthwise
Copy link

depthwise commented Nov 19, 2021

The unfortunate part about all this, I think, is that once you give your, say, 2 join handles to tokio::try_join!() they are moved and as such if one fails, you don't get to call abort() on the other even if that did in fact work.

Practical problem where this can arise is arguably fairly common - e.g. you want to have an HTTP endpoint and a GRPC endpoint in your program, or, say, HTTP REST and Prometheus instrumentation, or whatever - the key thing is two separate task must run until one of them fails, and be aborted if that happens, so the program could exit.

That said, I'm very much a beginner in Rust, so maybe I'm just "holding it wrong".

@Darksonn
Copy link
Contributor

Well yes, it's a tradeoff. When it comes to try_join!, you can pass a mutable reference to the join handle to not consume it.

@ghislaine-laios
Copy link

Is there any crate implementing the drop wrapper?

@najamelan
Copy link
Contributor

async_executors homogenizes the API across async frameworks and has a JoinHandle that aborts on drop by default.

@ghislaine-laios
Copy link

async_executors homogenizes the API across async frameworks and has a JoinHandle that aborts on drop by default.

Thanks.

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-proposal Category: a proposal and request for comments M-task Module: tokio/task
Projects
None yet
Development

No branches or pull requests