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

Proposal: Deprecate Tokio's LocalSet #6741

Open
Darksonn opened this issue Aug 1, 2024 · 6 comments
Open

Proposal: Deprecate Tokio's LocalSet #6741

Darksonn opened this issue Aug 1, 2024 · 6 comments
Labels
A-tokio Area: The main tokio crate C-proposal Category: a proposal and request for comments M-runtime Module: tokio/runtime

Comments

@Darksonn
Copy link
Contributor

Darksonn commented Aug 1, 2024

This is a proposal to deprecate the Tokio LocalSet abstraction in favor of a new LocalRuntime type. Currently, to spawn !Send tasks on Tokio you must use a combination of a current-thread runtime and LocalSet to make that work. However, tasks on a LocalSet are separate from the rest of the runtime in an uncomfortable way. For example:

  • Tasks on the LocalSet can tokio::spawn to get onto the runtime, but once you're on the runtime, you can no longer spawn_local to get back into the LocalSet.
  • From the runtime's perspective, all of the tasks in the LocalSet behave like a single task using FuturesUnordered, so various runtime options such as event_interval behave in a very surprising way by counting many tasks as a single task.
  • After discussions with Deno (a while ago), use of LocalSet has been shown to involve considerable performance overhead compared to unsafely spawning the !Send tasks onto a current-thread runtime.

Because of the above, I have proposed to add a new type called LocalRuntime which will be the replacement. Please see #6739 for the feature request to add that type. However, beyond just adding a new LocalRuntime type, I am also proposing to formally deprecate the LocalSet type. That's this issue.

Specifically, I am proposing the following course of action:

  1. First, we add a LocalRuntime type, which would close #6739.
  2. Then, 6 months later, we add #[deprecated] to the LocalSet type, so that existing users of LocalSet receive a warning that recommends LocalRuntime.
  3. Finally, since we are a stable 1.x crate, we keep LocalSet working forever.

I believe that all existing uses of LocalSet can be replaced either by the new LocalRuntime or by FuturesUnordered. The advantage of this approach is that the current design of LocalSet make it impossible to add some features without having them be really confusing:

  • Issue #3181 proposes to add hooks that run when a task is spawned / destroyed. The hooks would not run for tasks on the LocalSet. This is confusing.
  • Task dumps currently don't look inside block_on, which means that LocalSet tasks are completely invisible to task dumps.

In both cases, supporting LocalSet well is very difficult. As long as LocalSet remains a first-class citizen, it is difficult to introduce new features that affect all tasks without having confusing behavior when LocalSet is used.

Note that spawn_local is not being deprecated. The intent is that it will work together with LocalRuntime.

Some alternative options:

  • Do nothing. The previously mentioned disadvantages apply.
  • Add LocalRuntime but do not deprecate LocalSet. This provides a way to work around the performance issues of LocalSet, but ultimately it is still difficult to introduce new features that affect all tasks on the runtime.

The purpose of this issue is to gather feedback from users of LocalSet. Please post your experience below. I am especially interested in use-cases that cannot be replaced by LocalRuntime or FuturesUnordered.

@Darksonn Darksonn added C-proposal Category: a proposal and request for comments A-tokio Area: The main tokio crate M-runtime Module: tokio/runtime labels Aug 1, 2024
@Noah-Kennedy
Copy link
Contributor

Noah-Kennedy commented Aug 1, 2024

I am strongly in favor of this proposal.

@taikulawo

This comment was marked as resolved.

@Noah-Kennedy

This comment was marked as resolved.

@Noah-Kennedy
Copy link
Contributor

Anyone interested in following the LocalRuntime work can follow the PR (#6808) or the tracking issue (#6739).

@Sytten
Copy link

Sytten commented Sep 3, 2024

I am also in favour of this. The actix runtime is based on LocalSet which can bite you if you use tokio::spawn somewhere and that future uses actix::spawn. I will rewrite the actix_rt crate to use the new method once this is released.

@robjtede
Copy link
Contributor

robjtede commented Sep 4, 2024

On the actix-rt note, a better migration path for our downstream users would be if #[tokio::main] has this as a flavor option with or soon after release of LocalRuntime, making actix-rt an optional dependency strictly for actors support.

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-runtime Module: tokio/runtime
Projects
None yet
Development

No branches or pull requests

5 participants