runtime: merge multi & single threaded runtimes #1716

carllerche · 2019-10-31T20:54:50Z

Simplify Tokio's runtime construct by combining both Runtime variants
into a single type. The execution style can be controlled by a
configuration setting on Builder.

The implication of this change is that there is no longer any way to
spawn !Send futures. This, however, is a temporary limitation. A
different strategy will be employed for supporting !Send futures.

Included in this patch is a rework of task::JoinHandle to support
using this type from both the thread-pool and current-thread executors.

Simplify Tokio's runtime construct by combining both Runtime variants into a single type. The execution style can be controlled by a configuration setting on `Builder`. The implication of this change is that there is no longer any way to spawn `!Send` futures. This, however, is a temporary limitation. A different strategy will be employed for supporting `!Send` futures. Included in this patch is a rework of `task::JoinHandle` to support using this type from both the thread-pool and current-thread executors.

jonhoo · 2019-11-01T15:11:09Z

tokio/src/executor/task/core.rs

@@ -103,7 +106,7 @@ impl<T: Future> Core<T> {
        self.stage = Stage::Consumed
    }

-    pub(super) fn poll<S>(&mut self, header: &Header<S>) -> Poll<T::Output>
+    pub(super) fn poll<S>(&mut self, header: &Header) -> Poll<T::Output>


This change worries me, almost to the point that it should be marked as unsafe. Now, this basically allows doing an arbitrary pointer cast in safe code.

I don't think it is any more unsafe than it was before, but I can mark it as such.

Do you have a change in mind?

Before you had to have a Header<S> to go with the poll. Now, you no longer need to have a Header<S> to "prove" that you can use that S (before the proof was also arguably pretty weak). It's tricky, because I also agree with you argument from Discord that unsafe fn makes the whole function body an unsafe block, which isn't great. I think I'd be okay with this being discussed in a doc block for the fn instead.

tokio/src/executor/task/mod.rs

tokio/src/executor/task/harness.rs

tokio/src/runtime/builder.rs

jonhoo · 2019-11-01T15:30:16Z

tokio/src/runtime/builder.rs

-        let around_reactor_handles = reactor_handles.clone();
-        let around_timer_handles = timer_handles.clone();
+        // Blocking pool
+        let blocking_pool = PoolWaiter::from(Pool::default());


This doesn't seem right. The ThreadPool builder already creates a blocking pool, and stores a PoolWaiter internally. With this you'll end up with two pools.

You are correct. This was done intentionally as a transitional step. Fixing this "right" will take some restructuring of thread_pool that i want to punt to a follow up PR.

Okay, seems fine as long as it'll be fixed before the next release.

Maybe leave a TODO:?

jonhoo

Overall this looks good, though I left some notes about unsafety that I think should be documented better even if we're going to move things around later, and about what I believe to be a bug where a threadpool runtime ends up with two blocking pools.

tokio/src/runtime/builder.rs

hawkw · 2019-11-04T18:48:47Z

The implication of this change is that there is no longer any way to
spawn !Send futures. This, however, is a temporary limitation. A
different strategy will be employed for supporting !Send futures.

@carllerche can you share details on the "different strategy for supporting !Send futures"? I'd like to update tokio-compat to use the latest master, but in order to provide drop-in compatibility for the 0.1 current_thread runtime, we need a mechanism for spawning !Send futures. If you have a plan for the new approach, I'd love to help get support for spawning !Send futures back in.

carllerche · 2019-11-04T18:55:15Z

We can write something like LocalTaskSet that accepts !Send futures. This type can provide a spawn_local fn. Then, you would use it like:

let mut rt = Runtime::new();
let local_tasks = LocalTaskSet::new();
local_tasks.spawn(async { ... });

rt.block_on(local_tasks);

Not exactly that, but something like that should work.

s97712 · 2019-11-05T08:40:14Z

We can write something like LocalTaskSet that accepts !Send futures. This type can provide a spawn_local fn. Then, you would use it like:
let mut rt = Runtime::new();
let local_tasks = LocalTaskSet::new();
local_tasks.spawn(async { ... });

rt.block_on(local_tasks);
Not exactly that, but something like that should work.

Maybe we can provide a spawn_local method for runtime, spawn_local will spawn task on current thread. If the runtime has only one thread , then spawn and spwan_local have the same effect.

@carllerche

## Motivation In earlier versions of `tokio`, the `current_thread::Runtime` type could be used to run `!Send` futures. However, PR #1716 merged the current-thread and threadpool runtimes into a single type, which can no longer run `!Send` futures. There is still a need in some cases to support futures that don't implement `Send`, and the `tokio-compat` crate requires this in order to provide APIs that existed in `tokio` 0.1. ## Solution This branch implements the API described by @carllerche in #1716 (comment). It adds a new `LocalSet` type and `spawn_local` function to `tokio::task`. The `LocalSet` type is used to group together a set of tasks which must run on the same thread and don't implement `Send`. These are available when a new "rt-util" feature flag is enabled. Currently, the local task set is run by passing it a reference to a `Runtime` and a future to `block_on`. In the future, we may also want to investigate allowing spawned futures to construct their own local task sets, which would be executed on the worker that the future is executing on. In order to implement the new API, I've made some internal changes to the `task` module and `Schedule` trait to support scheduling both `Send` and `!Send` futures. Signed-off-by: Eliza Weisman <eliza@buoyant.io>

carllerche added 8 commits October 31, 2019 13:17

fix docs

c86abf2

try to fix CI

47fd91a

handle feedback

5e1039d

apply feedback

ffe988a

Merge remote-tracking branch 'origin/master' into simplify-rt

a496ac5

fmt

88c0604

crossbeam-channel is no longer needed

55e7c11

jonhoo reviewed Nov 1, 2019

View reviewed changes

tokio/src/executor/task/mod.rs Show resolved Hide resolved

jonhoo reviewed Nov 1, 2019

View reviewed changes

tokio/src/executor/task/mod.rs Outdated Show resolved Hide resolved

jonhoo reviewed Nov 1, 2019

View reviewed changes

tokio/src/executor/task/harness.rs Outdated Show resolved Hide resolved

jonhoo reviewed Nov 1, 2019

View reviewed changes

tokio/src/runtime/builder.rs Show resolved Hide resolved

jonhoo reviewed Nov 1, 2019

View reviewed changes

jonhoo requested changes Nov 1, 2019

View reviewed changes

apply feedback

5804a04

jonhoo approved these changes Nov 1, 2019

View reviewed changes

carllerche added 2 commits November 1, 2019 11:11

fix feature flags

d03ef6d

fmt

3514c51

jonhoo reviewed Nov 1, 2019

View reviewed changes

tokio/src/runtime/builder.rs Show resolved Hide resolved

carllerche added 3 commits November 1, 2019 11:37

woops, dev deps were deleted

58fb07f

Merge remote-tracking branch 'origin/master' into simplify-rt

90c4d31

Merge remote-tracking branch 'origin/master' into simplify-rt

6e31b41

carllerche merged commit d70c928 into master Nov 1, 2019

carllerche deleted the simplify-rt branch November 1, 2019 21:06

hawkw mentioned this pull request Nov 5, 2019

add task::LocalSet API for running !Send futures #1733

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

runtime: merge multi & single threaded runtimes #1716

runtime: merge multi & single threaded runtimes #1716

carllerche commented Oct 31, 2019

jonhoo Nov 1, 2019

carllerche Nov 1, 2019

carllerche Nov 1, 2019

jonhoo Nov 1, 2019

jonhoo Nov 1, 2019

carllerche Nov 1, 2019

jonhoo Nov 1, 2019

jonhoo Nov 1, 2019

jonhoo left a comment

hawkw commented Nov 4, 2019

carllerche commented Nov 4, 2019

s97712 commented Nov 5, 2019

runtime: merge multi & single threaded runtimes #1716

runtime: merge multi & single threaded runtimes #1716

Conversation

carllerche commented Oct 31, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonhoo left a comment

Choose a reason for hiding this comment

hawkw commented Nov 4, 2019

carllerche commented Nov 4, 2019

s97712 commented Nov 5, 2019