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

Updated to new tokio API #183

Merged
merged 3 commits into from
Jan 24, 2019
Merged

Conversation

dodomorandi
Copy link
Contributor

This should be helpful to introduce new async features, and maybe it
could ease the migration to the currently unstable futures.

I had to change the name of the feature from 'tokio' to 'async', because we cannot have crates and features with the same name.

Any feedback is appreciated!

This should be helpful to introduce new async features, and maybe it
could ease the migration to the currently unstable futures.
@alexcrichton
Copy link
Member

Thanks for this! I'd be somewhat wary though about changing names of the feautres, but tokio is a facade I think so perhaps smaller components could be used?

@dodomorandi
Copy link
Contributor Author

Ups, you are totally right! 😅 I will revert back the feature name and I will use the sub-crates of tokio instead of the whole facade.

* Using `tokio-io` as dependency instead of the whole `tokio` crate
* Reverted back to `tokio` for the feature name (because `tokio` crate
  is not imported anymore)
* Using `tokio-tcp` and `tokio-threadpool` for the test
@dodomorandi
Copy link
Contributor Author

A small note: I arbitrarily decided to use tokio-threadpool in order to spawn the futures in the test. The reason is just for consistency with tokio::run.

It should be possible to use tokio-current-thread if we assess that it is better for the tests.

@alexcrichton
Copy link
Member

Thanks! I think now this PR is largely just updating some idioms, having stricter requirements on the tokio-io and futures crates, and updating tests, right? If so I think it's safe to skip the version bump here as I don't think it'll be critical to get out to crates.io

@dodomorandi
Copy link
Contributor Author

I was not sure about the version bump, because of the old `tokio-core' and a few differences with the current environment. At the end I bumped just because if we forgot about it and merged, if could be a mess.

Surely the TcpStream API changed and both tokio::core and the try_nb macro are not available in newer versions. What do you recommend to evaluate if those are breaking changes for existent projects?

@alexcrichton
Copy link
Member

I think that tokio-core is only used during this crate's own tests and not used externally though, so the changes for users on crates.io are just internal rewrites. Otherwise the crate both before and after should compile with the same set of compilers and crates.

@dodomorandi
Copy link
Contributor Author

Perfect! If it is ok, I can change the version bump to 1.0.7, or I can revert it back to 1.0.6 if you would rather to include some other changes before the new version.

@alexcrichton
Copy link
Member

Since this is now largely a cleanup PR I'd prefer to leave it at 1.0.6 and we can let these changes ride to crates.io on the next bump

@alexcrichton alexcrichton merged commit d06d479 into rust-lang:master Jan 24, 2019
@alexcrichton
Copy link
Member

Thanks!

@dodomorandi dodomorandi deleted the new_tokio branch February 4, 2019 16:26
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