search

http-rs / surf

Fast and friendly HTTP client framework for async Rust
https://docs.rs/surf
Apache License 2.0
1.46k stars 119 forks source link

Add 'h1-client-rustls' feature relying on rustls #271

Closed JEnoch closed 3 years ago

JEnoch commented 3 years ago

This PR is a follow-up of http-rs/http-client#53 and addresses #40. It adds a h1-client-rustls feature using http-client/h1_client_rustls feature introduced by http-rs/http-client#53.

TODO: For testing purpose I set the dependency to http-client to directly use its Git repo. Once http-client is released with http-rs/http-client#53, update the dependency to use the latest version.

JEnoch commented 3 years ago

CI failures for curl-client feature are caused by http-rs/http-client#57. The workflow should be re-started after merge of http-rs/http-client#58

Fishrock123 commented 3 years ago

I will look more at the failures soon, but I think this would perhaps be better named as "h1-rustls-client"?

Ideally I'd like to separate out the tls backend flags... hyper has the same thing, I think. I haven't been able to figure out a good way to do that though...

JEnoch commented 3 years ago

@Fishrock123 I don't see such separation in hyper's features. And I'm not sure how to achieve this:

I don't think Cargo provides a way to declare such "feature replacement in case of another feature usage".

Thinking further, a solution could be such reorg in http-client features:

[features]
default = ["h1_client", "async-native-tls"]
h1_client = ["async-h1", "async-std"]
h1_rustls = ["async-tls"]

And re-exposing the same in Surf.

A user wanting h1 + rustls would use such dependency:

surf = { version = "2.1.0", default-features = false, features = ["h1-client", "h1-rustls"] }

The drawback is that using h1-client as only feature will fail to build since a TLS impl is missing. This would be a regression wrt. current behaviour.

What do you think ?

Fishrock123 commented 3 years ago

I don't see such separation in hyper's features.

reqwest, the hyper client wrapper, has this through. Their feature flags are pretty comprehensive. Maybe there's something to learn from them: https://github.com/seanmonstar/reqwest/blob/bd9ff9f371b756decf26fbbde1687433b0f63774/Cargo.toml#L30-L41

Fishrock123 commented 3 years ago

Thinking further, a solution could be such reorg in http-client features:

I also think we should re-organize http-client features. It's a big reason i never did another http-client release yet, because I wasn't sure what to do.

The drawback is that using h1-client as only feature will fail to build since a TLS impl is missing. This would be a regression wrt. current behaviour.

This actually isn't so bad, we can make http-client compile without tls support if nothing is provided.

Ideally, I think, http-client would have native-tls and rustls features, but this is a problem because cargo has no "join" on features e.g. make h1_client+native-tls pull in something different than maybe curl_client+native-tls. But maybe this isn't an issue if both hyper and isahc just use the same tls implementations as us anyways?

Fishrock123 commented 3 years ago

I have created a zulip thread about this to discuss with the cargo team... https://rust-lang.zulipchat.com/#narrow/stream/246057-t-cargo/topic/co-dependant.20feature.20flag.20dependencies

Fishrock123 commented 3 years ago

Maybe we just need to stick with h1-client-rustls for now though... this is also blocking https://github.com/nlopes/libhoney-rust/pull/61

Fishrock123 commented 3 years ago

@JEnoch mind to rebase this?

Fishrock123 commented 3 years ago

It seems like https://github.com/rust-lang/cargo/issues/8832 will eventually allow h1-client+rustls-tls to work, but no timeline on when.

JEnoch commented 3 years ago

@JEnoch mind to rebase this?

Sure! Doing this shortly.

JEnoch commented 3 years ago

Note that this PR relies on main branch of http-client (for http-rs/http-client#53 and http-rs/http-client#58). You might want to release http-client before merging this PR. Just let me know and I'll update the Cargo.toml.

Fishrock123 commented 3 years ago

I've released http-client 6.3.0

Fishrock123 commented 3 years ago

Merged a working set of features as https://github.com/http-rs/surf/pull/292, which is more like the original without my poor suggestion on feature separation (which is not yet supported by cargo in the way which we'd need).

Thanks for your work and patience here!