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

executor: add TypedExecutor #993

Merged
merged 14 commits into from
Mar 21, 2019
Merged

executor: add TypedExecutor #993

merged 14 commits into from
Mar 21, 2019

Conversation

carllerche
Copy link
Member

Add a TypedExecutor. The trait is similar to futures::Executor but with an error type matching Executor.

While the trait is similar enough to the one in futures, Tokio will offer long term support for it (vs, the one in futures-rs going away).

This trait is the basis for implementing components that require spawning and wish to be generic over executors that supports Send futures vs. ones that do not. See here.

Remaining

  • tests
  • docs

Closes #625

pub trait TypedExecutor<T> {

/// TODO: DOX
fn typed_spawn(&mut self, future: T) -> Result<(), SpawnError>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a minor preference for naming this method spawn.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opted for typed_spawn to avoid name conflicts since most types that impl Executor will also implement TypedExecutor. Still prefer spawn?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I figured that's why you did, but I expect only 1 trait would be imported, and so typed_spawn reads weird.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm ok w/ switching it to spawn.

tokio-executor/src/executor.rs Show resolved Hide resolved
@@ -21,3 +21,6 @@ members = [
"tokio-udp",
"tokio-uds",
]

[patch.crates-io]
tokio-executor = { path = "tokio-executor" }
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prevent sub-crates that do not depend on the change from pulling in tokio-executor from crates.io.


# Remove any existing patch statements
mv Cargo.toml Cargo.toml.bck
sed -n '/\[patch.crates-io\]/q;p' Cargo.toml.bck > Cargo.toml
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Handle the case when the primary Cargo.toml file has a patch section already. This is now extracted into a dedicated file in order to reuse from multiple spots.

/// * An executor has been shutdown and can no longer accept new futures. This
/// error state is expected to be permanent.
#[derive(Debug)]
pub struct SpawnError {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SpawnError is moved into its own file. No changes are made.

use futures::Future;
use SpawnError;

/// A value that executes futures.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Executor is moved into its own file. No changes are made.

@carllerche carllerche marked this pull request as ready for review March 21, 2019 20:57
@carllerche carllerche merged commit b1172f8 into master Mar 21, 2019
carllerche added a commit that referenced this pull request Mar 22, 2019
#993 introduces changes in a sub crate that other Tokio crates depend
on. To make CI pass, a `[patch]` statement and `path` dependencies are
used.

When releasing, these must be removed. However, the commit that
removes them and prepares the crates for release will not be able to
pass CI.

This commit adds a conditional on a special `[ci-release]` snippet in
the commit message. If this exists, CI is only run with the full "patched"
dependencies.
@carllerche carllerche deleted the executor-reform branch March 22, 2019 20:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants