Document that the Connector can determine HTTP version

[jyn514]

Version
hyper 0.14.14

Platform
Linux pop-os 5.13.0-7614-generic #14~1631647151~20.04~930e87c-Ubuntu SMP Fri Sep 17 00:26:31 UTC x86_64 x86_64 x86_64 GNU/Linux

Description

Hyper silently upgrades HTTPS requests that set Version::HTTP_11 to an HTTP/2 connection.

I tried this code:

    // set up https client to connect to the preview service
    let https = HttpsConnector::with_native_roots();
    let client = HyperClient::builder().build::<_, Body>(https);

    // create a closure that hyper will use later to handle HTTP requests
    let make_service = make_service_fn(move |_| {
        let client = client.to_owned();

        async move {
            Ok::<_, anyhow::Error>(service_fn(move |req| {
                let client = client.to_owned();
                *req.uri_mut() = "https://example.com".parse().unwrap();
                *req.version_mut() = Version::HTTP_11;
                async move {
                    let mut resp = match client.request(req).await {
                        Ok(r) => r,
                        Err(err) => {
                            panic!("{}", err);
                        }
                    };

I expected to see this happen: Hyper sends an HTTP/1.1 request.
Instead, this happened: Hyper sends an HTTP/2 request.

You can verify this either with wireshark or by applying this diff, which will cause the above snippet to panic instead of sending the wrong version:

diff --git a/src/client/client.rs b/src/client/client.rs
index 72a78ab1..95810583 100644
--- a/src/client/client.rs
+++ b/src/client/client.rs
@@ -268,6 +268,11 @@ where
             } else {
                 origin_form(req.uri_mut());
             }
+        } else if pooled.is_http2() && req.version() != Version::HTTP_2 {
+            warn!("Connection is HTTP/2, but request requires HTTP/1");
+            return Err(ClientError::Normal(
+                crate::Error::new_user_unsupported_version(),
+            ));
         } else if req.method() == Method::CONNECT {
             authority_form(req.uri_mut());
         }

This matters when proxying websocket connections, which are only supported with HTTP/2 when the server supports the RFC 8441 extension.

[seanmonstar]

Ah, I see what you mean. Even if the pool didn't have an existing HTTP/2 connection, it looks like the connector is probably using ALPN to negotiate h2 in the handshake. I was going to say something like in #2605 might help here, but only if we also had a way to tell the connector "don't ask for h2"...

[jyn514]

but only if we also had a way to tell the connector "don't ask for h2"...

right - there's an http2_only function on Client, but no corresponding http1_only. I actually worked around this by enabling HTTP/1 in the TLS connector, but it was confusing to me both a) that the decision to use HTTP/1 or HTTP/2 happens there, and b) that Hyper will silently use its decision even if it doesn't match the protocol of the request.

cloudflare/wrangler-legacy#2153

[jyn514]

@nox had some ideas of how hyper could improve this too, but I don't quite remember what they were ...

[seanmonstar]

that the decision to use HTTP/1 or HTTP/2 happens there

Where is there in this case? The connector?

Hyper will silently use its decision even if it doesn't match the protocol of the request.

It's more that hyper needs to use the decision from the connector. If a protocol was negotiated via ALPN (either h2 or http/1.1), then that's the protocol that MUST be used on the connection. So in that sense, what the connector says is "final".

The version specified in the Request is a hint, but configuration on the Client or connector have stronger weight. This is also true in other projects, like curl. Perhaps we could improve the documentation about that?

[jyn514]

Where is there in this case? The connector?

Yes, in the TLS connector.

Perhaps we could improve the documentation about that?

That would be great! It wasn't clear to me where the decision was happening.