Consider adding a TypedExecutor trait

[carllerche]

Motivation

The current executor trait requires that spawned tasks are Send. This prevents libraries from being able to spawn while supporting the !Send use case.

Proposal

The following trait would be added to tokio-executor:

pub trait TypedExecutor<F: Future<Item = (), Error = ()>> {
    fn execute(&self, future: F) -> Result<(), SpawnError>;

   fn status(&self) -> Result<(), SpawnError>;
}

This trait would be in addition to the curreent Executor trait.

The concurrent runtime would have the following implementation:

impl<T: Future<Item = (), Error = ()> + Send> TypedExecutor<T> for Runtime { ... }

The currrent_thread runtime would have:

impl<T: Future<Item = (), Error = ()>> TypedExecutor<T> for Runtime { ... }

Then, libraries that require spawning tasks would be able to be generic over this TypedExecutor:

struct MyType<T, E> {
    executor: E,
    thing: T,
}

struct MyTask<T> { ... }

impl<T, E> MyType<T, E>
where E: TypedExecutor<MyTask<T>>,
{
    pub fn new(thing: T, executor: E) -> Self { ... }
}

This way, users of the library are not forced to make T: Send.

Open questions

• Should there be blanket implementations between Executor and TypedExecutor? If so, what direction should they go?

• Should there also be a LocalExecutor trait is the same as Executor but accepts tasks that are not !Send?

Follow up

Add spawn_handle: #638

Refs: #446, hyperium/hyper#1670

[carllerche]

I'm wondering if, instead of adding a new trait, we could make the current Executor trait work for all cases:

pub trait Executor<T = Box<Future<Item = (), Error = ()> + Send>> {
    fn spawn(&mut self, future: T) -> Result<(), SpawnError>;
}

I also think that this would be backwards compatible.

@hawkw could you try making the above change and see if our stack compiles? (we need our own crater).

[hawkw]

I've made the change @carllerche described in #625 (comment) in a branch, and so far, it at least seems to be backwards-compatible within tokio --- all the crates in this repo that depend on tokio-executor still compile.

I'm going to do some additional testing against the rest of the stack, such as tower.

My two cents: I think this is an excellent idea --- potentially we can reduce a lot of code duplication, hopefully without making a breaking API change.

[hawkw]

An update: I've tested the branch I linked above against tower and tower-h2 (both of which only depend on tokio-executor for their tests) and linkerd2-proxy. All these codebases work with this change without any apparent issues, so it does seem to be backwards-compatible.

As a side note, tower-grpc's tests still depend on tokio-core, so I didn't try patching it to use this branch.

[carllerche]

@seanmonstar has made good progress figuring out how this would fit together in a library.

Libraries like hyper need to spawn tasks that are composed of futures provided by the user. Hyper would like to be able to use the threaded runtime when the user tasks are Send, but allow !Send tasks by using the current_thread Runtime. However, Hyper does not wish to expose its internal task types.

The proposed strategy for this case is as follows:

Assuming that a library takes a user provided future of type F. The library defines its task as:

struct MyLibTask<T> { ... }

impl<T> Future for MyLibTask<T> { ... }

The MyLibTask is not exposed as part of the public API.

The library then defines a new trait, with a blanket implementation. The trait is part of the library's public API.

use tokio_executor::TypedExecutor;

/// Documentation explaining that the user should pass in either Tokio's threaded
/// runtime's executor or the current_thread's runtime's executor depending on if
/// `T` is `Send` or `!Send`.
pub trait MyLibExecutor<T>: TypedExecutor<MyLibTask<T>> { }

impl<T, E: TypedExecutor<MyLibTask<T>> MyLibExecutor<T> for E { }

Then, the library accepts E: MyLibExecutor<T> as part of its public API when it takes a user supplied executor:

impl Builder {
    fn executor<T, E: MyLibExecutor<T>>(self, executor: E) -> Self { ... }
}

// Usage
my_lib::Builder::new()
    .executor(exec)
    .build()

[jonhoo]

Maybe I'm missing something, but the proposed solution still doesn't quite hide MyLibTask? The E: TypedExecutor<MyLibTask<T> bound still names MyLibTask. I guess for a TypedExecutor<T> that has an impl for something like all T: Future it's fine, but if it doesn't have that blanket impl, users would still have to explicitly name MyLibTask.

nvm: the blanket impl is private. we do prevent users from implementing their own custom executor that only takes certain futures, but that's probably not so important.

[DoumanAsh]

Should there also be a LocalExecutor trait is the same as Executor but accepts tasks that are not !Send?

If TypedExecutor would generic over task type, then I do not think there is need for explicit local executor.
Being able to specify trait bounds for task, would make it enough to distinguish I think?

I'd say I'm looking forward to see this TypedExecutor to replace existing one