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
Enable ALPN for native-tls users #1283
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I agree it'd be awesome to get this in, comments inline.
Cargo.toml
Outdated
@@ -30,7 +30,7 @@ default = ["default-tls"] | |||
|
|||
# Note: this doesn't enable the 'native-tls' feature, which adds specific | |||
# functionality for it. | |||
default-tls = ["hyper-tls", "native-tls-crate", "__tls", "tokio-native-tls"] | |||
default-tls = ["hyper-tls", "native-tls-crate", "native-tls-crate/alpn", "__tls", "tokio-native-tls"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think enabling this by default could be a breaking change, because it will try to access new symbols in the linked libraries that may not exist. I wish the feature could be made to check at runtime, but it currently doesn't.
So, we'd probably need a separate native-tls-alpn
feature in reqwest. Unless there's a better option?
src/connect.rs
Outdated
@@ -687,7 +691,16 @@ mod native_tls_conn { | |||
|
|||
impl<T: Connection + AsyncRead + AsyncWrite + Unpin> Connection for NativeTlsConn<T> { | |||
fn connected(&self) -> Connected { | |||
self.inner.get_ref().get_ref().get_ref().connected() | |||
if self.inner.get_ref().negotiated_alpn().ok() == Some(Some(b"h2".to_vec())) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will allocate a small vector before doing the comparison, we could just compare the slices with some more pattern matching.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wonderful! Wanna add a note to the main doc page (in lib.rs
) about the new feature, right between native-tls-vendored
and rustls-tls
?
* master: add option to disable http2 upgrade (seanmonstar#1292) Fix documentation of http1_title_case_headers() (seanmonstar#1294) CI: make a single final job that depends on all others (seanmonstar#1291) Fix bare url warnings in `multipart` docs (seanmonstar#1289) v0.11.4 Allow overriding of DNS resolution to specified IP addresses(seanmonstar#561) (seanmonstar#1277) WASM: Add `try_clone` implementations to `Request` and `RequestBuilder` (seanmonstar#1286) Add native-tls-alpn feature (seanmonstar#1283)
Thanks to sfackler/rust-native-tls#194, native-tls v0.2.7 now supports ALPN. This should allow native-tls users to initiate an HTTP2 connection without prior knowledge.
Resolves remaining bits of #292