Add a generic `Executor` trait #455

alexcrichton · 2017-04-21T19:33:00Z

This commit adds a new trait to the future module, Executor. This trait has
only one method:

trait Execute<F: Future<Item = (), Error = ()>> {
    fn execute(&self, f: F) -> Result<(), ExecuteError<F>>;
}

The purpose of this trait is to unify the various executors found throughout the
ecosystem. Crates which require the ability to spawn futures will now have the
option ot operate generically over all Executor instances instead of a
particular executor.

A Future::background method was also added to ergonomically pass a future to
an executor and run it in the background. The two oneshot modules (sync and
unsync) also grew spawn and spawn_fn functions which will spawn a future
onto an executor, returning a handle to the resulting future. This can be used
to spawn work onto a specific executor and still get notified when the work
itself is completed.

Finally, an implementation of the Executor trait was added to CpuPool. Due
to the differences in unwinding/panic semantics, though, the CpuPool::spawn
method which previously existed was not deprecated.

Closes #313

alexcrichton · 2017-04-21T19:33:47Z

I was pretty happy with everything until I ended up hitting CpuPool and the differences in panicking semantics. Overall though I'm pretty happy with how this turned out. I'd still like to play around with this in tokio-core though before landing.

r? @carllerche

carllerche

Looks good at first glance. Just the Result return value comment.

carllerche · 2017-04-21T20:46:27Z

src/future/mod.rs

+    /// This function will return immediately, and schedule the future `future`
+    /// to run on `self`. The details of scheduling and execution are left to
+    /// the implementations of `Spawn`.
+    fn spawn(&self, future: F);


This function should return Result<(), Error<F>> to allow the executor to return the future if it fails to spawn it.

The docs should say that returning the future on error is "best effort" and returning Ok is not a guarantee that the future will be executed.

Oops forgot that! I've updated with the error

carllerche · 2017-04-24T17:21:03Z

Originally experimentation: https://github.com/carllerche/futures-spawn, though the latest is on a branch https://github.com/carllerche/futures-spawn/tree/restructure.

Once this PR is merged, I need to remember to deprecate that crate (leaving this comment to remind me!)

bluetech · 2017-05-03T14:10:13Z

I saw this PR, and wanted to see if I understand the goal correctly (I hope you don't mind).

Consider a Hyper tokio server for example. Currently, hyper depends on tokio-core and tokio-proto -> tokio-core. If I understand this trait correctly it can help with removing this dependency -- instead of taking Core/Handle, it will take this trait. Then it should be possible to run a Hyper server with e.g.

A tokio-core/mio event loop
A cached thread pool (CpuPool)
Any other Spawn implementation

by just changing a few lines in the application code (the library is agnostic).

To test this assumption and check if it's viable, I tried to do the following. First in Hyper:

Removed all client/ code (irrelevant for this discussion)
Removed the helper Server trait, leaving only Http to use directly (Server uses tokio-core so I'm just removing it from consideration for now).
Replaced all uses of Handle with an appropriate Spawn bound

and then in tokio-proto:

Removed tcp_client/tcp_server - not used by hyper so ignoring that for now.
Removed all of mentions of old_io (backward compatibility support for tokio_core::io, replaced by tokio_io).
Replaced all uses of Handle with an appropriate Spawn bound

In the last bullet it got messy and I ran out of time to play with it, so I didn't reach a conclusion. But I think that it can be done - am I correct?

If it would be possible to statically parametrize a Hyper server (as an example) over a Spawn trait, and it would all work flawlessly when they are switched, I think it would really show the strength of the abstractions here - it would be quite a feat :)

alexcrichton · 2017-05-03T18:17:54Z

@bluetech you are indeed spot on! That's the intended use case, decoupling crates like Hyper from tokio-core specifically and allowing them to interoprate more gracefully with other crates like futures-glib. The Spawn trait isn't sufficient for everything tokio-proto and hyper use, but everything minus the conveniences are sufficient to use Spawn (or at least that's the intention).

leoyvens · 2017-05-20T18:05:51Z

Name conflict with the Spawn struct seems bad. Executor seems like the right name here, we should steal that name from the current Executor trait and rename that (to Dispatch perhaps?). It is an unfortunate name anyway since when the docs say executor they mean something that spawns futures rather than something that actually implements the current Executor trait.

Going maybe out of scope from this PR, the current Executor starts looking like an odd duck since it is an executor but doesn't implement Spawn. Maybe we should make the current Executor into a struct that implements Spawn trait, and deprecate the execute method from the Spawn struct. It dosen't fit well anyways since task.execute(executor) reads backwards.

The renames would go like:
Executor trait -> Dispatch trait
Spawn trait -> Executor trait

Refactoring Dispatch:
Create a Dispatcher struct that wraps Arc<Dispatch>.
Dispatcher implements new Executor trait (the trait proposed here as Spawn).
Deprecate Spawn::execute from the Spawn struct.

alexcrichton · 2017-05-21T15:37:58Z

@leodasvacas that sounds pretty reasonable to me! I definitely think the current Executor trait is basically deprecated and should likely be replaced by this.

@carllerche what do you think about the naming here?

carllerche · 2017-05-22T18:06:38Z

Renaming seems fine. I would rename the trait fn to match the trait name.

Executor is deprecated, so reusing the name seems fine.

Other names:

Schedule
Run.

I don't have a huge preference when it comes to the actual name. I do think avoiding the Spawn conflict seems good.

alexcrichton · 2017-05-24T16:06:38Z

Ok I've updated to Executor. r? @carllerche?

leoyvens · 2017-05-24T18:26:30Z

Nice! The fate of "OldExecutor" will be decided later? background is ergonomic and I'm not against it, but there are some points to be discussed:

It's offering both a panicking and non-panicking version of the same API, std generally avoids this with a preference towards non-panicking.
Naming: I think we should just call it execute_on for consistency and neutrality (the executor may block for all we know).

carllerche

Seems mostly good, I had one comment re: panicking (aka, I also think it should be avoided). I personally am leaning towards just dropping the error in the "convenience" function.

carllerche · 2017-05-24T20:17:36Z

src/future/mod.rs

+        where E: Executor<Self>,
+              Self: Future<Item = (), Error = ()> + Sized,
+    {
+        executor.execute(self).expect("failed to execute future in background")


I agree that we should avoid panicking. An executor being unable to execute a task is a very plausible situation to get into.

The two options are for background to return a Result OR for it to silently drop the error.

I'm leaning towards just dropping the error because:

a) background is a convenience API
b) There is no guarantee that an executor returns an error on execution failure. It's completely acceptable for the executor to silently drop the task as well.

alexcrichton · 2017-05-25T19:35:45Z

Ok I've updated to just delete background for now.

@leodasvacas I'd like to deprecate the old Executor trait as part of this. I meant to investigate your suggestion a bit more but I'll go do that now.

This commit adds a new trait to the `future` module, `Executor`. This trait has only one method: trait Execute<F: Future<Item = (), Error = ()>> { fn execute(&self, f: F) -> Result<(), ExecuteError<F>>; } The purpose of this trait is to unify the various executors found throughout the ecosystem. Crates which require the ability to spawn futures will now have the option ot operate generically over all `Executor` instances instead of a particular executor. The two `oneshot` modules (sync and unsync) also grew `spawn` and `spawn_fn` functions which will spawn a future onto an executor, returning a handle to the resulting future. This can be used to spawn work onto a specific executor and still get notified when the work itself is completed. Finally, an implementation of the `Executor` trait was added to `CpuPool`. Due to the differences in unwinding/panic semantics, though, the `CpuPool::spawn` method which previously existed was not deprecated. Closes #313

alexcrichton · 2017-05-25T19:41:15Z

Ok I've commented that we're likely to deprecate executor::Executor and Spawn::execute but I haven't done so just yet. I don't think we have tons of users of that trait so I don't want to agonize too much over names just yet. in that case I'm going to merge!

alexcrichton mentioned this pull request Apr 21, 2017

Consider providing a Spawn trait #313

Closed

alexcrichton requested a review from carllerche April 21, 2017 20:40

carllerche reviewed Apr 21, 2017

View reviewed changes

GGist mentioned this pull request Apr 23, 2017

Create Trait To Abstract Handle/RemoteHandle GGist/bip-rs#71

Open

alexcrichton force-pushed the spawn-trait branch from bca0662 to f2acbd6 Compare April 24, 2017 16:30

alexcrichton force-pushed the spawn-trait branch from f2acbd6 to 5eafcb6 Compare May 3, 2017 18:16

carllerche mentioned this pull request May 8, 2017

CpuPool::spawn_stream #470

Closed

alexcrichton mentioned this pull request May 20, 2017

Missing event loop understanding documentation tokio-rs/tokio#9

Closed

alexcrichton force-pushed the spawn-trait branch from 5eafcb6 to 9f35221 Compare May 24, 2017 16:05

alexcrichton changed the title ~~Add a generic Spawn trait~~ Add a generic Executor trait May 24, 2017

carllerche reviewed May 24, 2017

View reviewed changes

alexcrichton force-pushed the spawn-trait branch from 9f35221 to 2668477 Compare May 25, 2017 19:34

alexcrichton force-pushed the spawn-trait branch from 2668477 to 0ed8fc5 Compare May 25, 2017 19:40

alexcrichton merged commit b24dea1 into master May 25, 2017

alexcrichton deleted the spawn-trait branch May 25, 2017 19:41

alexcrichton mentioned this pull request May 25, 2017

Consider having remote.spawn return a future w/ the result tokio-rs/tokio-core#77

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a generic `Executor` trait #455

Add a generic `Executor` trait #455

alexcrichton commented Apr 21, 2017 •

edited

Loading

alexcrichton commented Apr 21, 2017

carllerche left a comment

carllerche Apr 21, 2017

alexcrichton Apr 24, 2017

carllerche commented Apr 24, 2017

bluetech commented May 3, 2017

alexcrichton commented May 3, 2017

leoyvens commented May 20, 2017

alexcrichton commented May 21, 2017

carllerche commented May 22, 2017

alexcrichton commented May 24, 2017

leoyvens commented May 24, 2017

carllerche left a comment

carllerche May 24, 2017

alexcrichton commented May 25, 2017

alexcrichton commented May 25, 2017

Add a generic Executor trait #455

Add a generic Executor trait #455

Conversation

alexcrichton commented Apr 21, 2017 • edited Loading

alexcrichton commented Apr 21, 2017

carllerche left a comment

Choose a reason for hiding this comment

carllerche Apr 21, 2017

Choose a reason for hiding this comment

alexcrichton Apr 24, 2017

Choose a reason for hiding this comment

carllerche commented Apr 24, 2017

bluetech commented May 3, 2017

alexcrichton commented May 3, 2017

leoyvens commented May 20, 2017

alexcrichton commented May 21, 2017

carllerche commented May 22, 2017

alexcrichton commented May 24, 2017

leoyvens commented May 24, 2017

carllerche left a comment

Choose a reason for hiding this comment

carllerche May 24, 2017

Choose a reason for hiding this comment

alexcrichton commented May 25, 2017

alexcrichton commented May 25, 2017

Add a generic `Executor` trait #455

Add a generic `Executor` trait #455

alexcrichton commented Apr 21, 2017 •

edited

Loading