-
Notifications
You must be signed in to change notification settings - Fork 611
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
Add a generic Executor
trait
#455
Conversation
I was pretty happy with everything until I ended up hitting r? @carllerche |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good at first glance. Just the Result
return value comment.
src/future/mod.rs
Outdated
/// 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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops forgot that! I've updated with the error
bca0662
to
f2acbd6
Compare
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!) |
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
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:
and then in
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 |
@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 |
Name conflict with the Going maybe out of scope from this PR, the current The renames would go like: Refactoring |
@leodasvacas that sounds pretty reasonable to me! I definitely think the current @carllerche what do you think about the naming here? |
Renaming seems fine. I would rename the trait fn to match the trait name.
Other names:
I don't have a huge preference when it comes to the actual name. I do think avoiding the |
Ok I've updated to |
Nice! The fate of "OldExecutor" will be decided later?
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
src/future/mod.rs
Outdated
where E: Executor<Self>, | ||
Self: Future<Item = (), Error = ()> + Sized, | ||
{ | ||
executor.execute(self).expect("failed to execute future in background") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Ok I've updated to just delete @leodasvacas I'd like to deprecate the old |
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
Ok I've commented that we're likely to deprecate |
This commit adds a new trait to the
future
module,Executor
. This trait hasonly one method:
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 aparticular executor.
A
Future::background
method was also added to ergonomically pass a future toan executor and run it in the background. The two
oneshot
modules (sync andunsync) also grew
spawn
andspawn_fn
functions which will spawn a futureonto 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 toCpuPool
. Dueto the differences in unwinding/panic semantics, though, the
CpuPool::spawn
method which previously existed was not deprecated.
Closes #313