fix(http client): use https connector for https #750
Conversation
niklasad1
changed the title
client/http-client/src/transport.rs
Outdated
| let client = match target.scheme_str() { | ||
| Some("http") => { | ||
| let mut connector = HttpConnector::new(); | ||
|
|
||
| connector.set_reuse_address(true); |
There was a problem hiding this comment.
remove these to more "similar" to `HTTPs connector?
There was a problem hiding this comment.
Does removing them have any obvious impact on anything? I did a bit of reading up on them and they both sound like optimisations anyway (I wasn't sure that nodelay would actually help anything much here; it sounds like it trades a little throughput for lower latency, and reuseaddress sounds like an optimisation around freeing up an address a little quicker mainly).
I'd vote to either remove them if no real difference is observed, or add a comment above them saying why they are important to have here. (I'd have thought you could set them for the https connector too but it wasn't so obvious to me how!)
There was a problem hiding this comment.
nah, no real difference might reduce the number of file descriptors for the benchmarks.
let's remove it.
| _ => return Err(Error::InvalidCertficateStore), | ||
| }; | ||
| let client = Client::builder().build::<_, hyper::Body>(connector); | ||
| HyperClient::Https(client) | ||
| HyperClient::Https(Client::builder().build::<_, hyper::Body>(connector)) |
There was a problem hiding this comment.
I'm not really sure what the impact of this change is; was wrapping the HttpConnector causing an issue?
There was a problem hiding this comment.
Ah I see; I just read rustls/hyper-rustls#169. Interesting!
My reading/assumption was that the wrapping would work as we had expected!
Closing #748
Wrapping the
HttpConnectorin theHttpsConnectordoesn't work, see for futher info rustls/hyper-rustls#169 which this PR fixes.