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

Optionally disable pulling in the tokio runtime #69

Merged
merged 1 commit into from
Mar 15, 2019
Merged

Conversation

erickt
Copy link
Contributor

@erickt erickt commented Mar 11, 2019

Before this patch, hyper-rustls required pulling in the tokio runtime due to it's use of hyper::client::HttpConnector. Unfortunately Fuchsia does not support tokio, which prevents our use of this
library. This PR allows us to optionally disable pulling in tokio, and instead swap in our own version of the hyper HttpConnector.

If this PR is accepted, would it be possible for a new version to be cut that allows us to opt out of the tokio dependency? Thanks so much!

@cramertj
Copy link
Contributor

Would you mind adding a no-default-features build to the Travis config to ensure this doesn't regress?

@lucab
Copy link
Contributor

lucab commented Mar 12, 2019

I'm a bit wary that making hyper an optional dependency of hyper-rustls is a good idea. I'll wait for @ctz to chime in.

instead swap in our own version of the hyper HttpConnector

I don't see it in this PR, so I'm not sure about the details of this. Why can't hyper directly host this dedicated connector flavor, instead of pushing a feature flag to rev-dependencies (including this crate)?

Before this patch, hyper-rustls required pulling in the tokio runtime
due to it's use of `hyper::client::HttpConnector`. Unfortunately
Fuchsia does not support tokio, which prevents our use of this
library. This PR allows us to optionally disable pulling in tokio,
and instead swap in our own version of the hyper `HttpConnector`.
@erickt
Copy link
Contributor Author

erickt commented Mar 12, 2019

@cramertj: good idea, I added it, and fixed the doctest to work with --no-default-features.

@lucab:

I'm a bit wary that making hyper an optional dependency of hyper-rustls is a good idea. I'll wait for @ctz to chime in.

Did you mean tokio? I'm not making hyper optional, I'm just allowing for the possibility of disabling hyper's runtime feature, which at the moment in tokio.

instead swap in our own version of the hyper HttpConnector

I don't see it in this PR, so I'm not sure about the details of this.

Our replacement for hyper's HttpConnector lives in the fuchsia tree. Our plan is that we'd write a small function to setup a hyper_rustls::HttpsConnector with a fuchsia_hyper::HyperConnector in our tree, rather than try to land that in hyper-rustls, because the Fuchsia APIs are still being changed fairly rapidly, and we don't want to expose you to unnecessary churn.

Why can't hyper directly host this dedicated connector flavor, instead of pushing a feature flag to rev-dependencies (including this crate)?

I'd love it if we could do that, but unfortunately I think this is all a consequence of the current state of things with rust futures. With stable futures, I don't think there's a great way to abstract away executors, which is why the Hyper runtime is pretty tightly bound to tokio. Abstracting exectors is better supported in futures-preview, but that's still nightly only. Since hyper wants to be compatible with stable rust, there really isn't an option that we can use.

However, std::task and std::future just stablized, so hopefully this situation will simplify soon.

@lucab
Copy link
Contributor

lucab commented Mar 12, 2019

Did you mean tokio? I'm not making hyper optional, I'm just allowing for the possibility of disabling hyper's runtime feature, which at the moment in tokio.

Nevermind, I initially misread the manifest change.

@erickt thanks for the followup and the references, with all the details I see the context and the trade-offs involved. I think it makes sense to land this now and hopefully re-unify Fuchsia support later on.

I'd still leave some time for @ctz to chime in, in case he has some thoughts on this.

@erickt
Copy link
Contributor Author

erickt commented Mar 15, 2019

@lucab or @ctz, just a friendly ping to see if this PR looks good, or if you need any other changes. Thanks!

@ctz
Copy link
Member

ctz commented Mar 15, 2019

This seems sensible to me.

@ctz ctz merged commit a94d2a9 into rustls:master Mar 15, 2019
@erickt erickt mentioned this pull request Mar 15, 2019
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.

4 participants