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

net/http: provide some Transport knob (bool?) to keep max 1 TLS dial in flight at once when discovering http2 support #25761

Open
hapnermw opened this Issue Jun 6, 2018 · 9 comments

Comments

Projects
None yet
2 participants
@hapnermw

hapnermw commented Jun 6, 2018

Please answer these questions before submitting your issue. Thanks!

What version of Go are you using (go version)?

go version go1.10.2 darwin/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

Container Linux by CoreOS stable (1745.5.0)

What did you do?

Attached NginX client replicates this issue. It requires some minimal reconfiguration for a test HTTP2 server. The client<->Nginx hop is HTTP2 and the NginX<->Service hop is HTTP1.1. This is a typical config. I've verified NginX is exposing an HTTP2 virtual server endpoint with the h2c utility.

Any HTTP2 client for an NginX HTTP2 reverse proxy should replicated this behavior.
NginX sets http2_max_concurrent_streams to 128 by default so it should accept the 3 concurrent streams in this test.

I've also included an equivalent HTTP2 client for http2.golang.org. httptrace reports this client does one TLS handshake per request; however, all three requests use the same connection.

So, for some reason not visible via httptrace, the client is multiplexing requests to a Go HTTP2 server; but, is not multiplexing requests to an NginX HTTP2 reverse proxy.

What did you expect to see?

I expected to see the go http client multiplex these concurrent HTTP2 requests on a single HTTP2 connection to the NginX HTTP2 reverse proxy.

I expected that only one TLS handshake would be done.

What did you see instead?

Instead of multiplexing these requests on one NginX reverse proxy connection, the client sends each request on a separate connection.

The duplicate TLS handshakes reported by httptrace for http2.golang.org don't make sense if all requests are using the same connection.

nginx.conf.txt
simSwitch.go.txt
http2client.go.txt
simSwitch-httptrace.txt
http2client-httptrace.txt

@bradfitz

This comment has been minimized.

Member

bradfitz commented Jun 6, 2018

If you immediately start 3 HTTP requests in parallel with no cached connections open in a fresh program, net/http doesn't know that your peer supports HTTP/2, so it does a new TCP connection & TLS handshake until it discovers the "h2" ALPN response.

If that's all this is about, it sounds like working as intended.

You might be interested in #6785 / #13957 (kinda dups), though, and https://go-review.googlesource.com/c/go/+/71272 which adds a limit on the number of connections per host, which includes those in dialing state. But neither of those is specifically about limiting per-host initial TLS dials for the purpose of discovering whether a host supports HTTP/2.

The problem with doing that by default is that you sacrifice latency in the case where it turns out the peer did NOT support HTTP/2. Now some of the requests have to then do the TCP connection after all with the associated TCP+TLS setup round trips.

@bradfitz bradfitz changed the title from HTTP2 client multiplex issue with NginX HTTP2 reverse proxy to net/http: provide some Transport knob (bool?) to keep max 1 TLS dial in flight at once when discovering http2 support Jun 6, 2018

@bradfitz bradfitz added this to the Unplanned milestone Jun 6, 2018

@hapnermw

This comment has been minimized.

hapnermw commented Jun 7, 2018

@bradfitz

This comment has been minimized.

Member

bradfitz commented Jun 7, 2018

Immediately executing three concurrent requests appears to work correctly
with the http2.golang.org server as I noted in my issue.

I think you might just be getting lucky with timing.

Bottom-line, there is one or more HTTP2 protocol issues here that need to
be sorted.

Which part of the spec are you referring to? I don't recall anything about this.

On the latency vs HTTP2 correct operation tradeoffs, if the latency for the
non-HTTP2 case is an issue then providing both options is needed. The
correct HTTP2 operation should be the default.

Maybe. But I think a number of other people might disagree on their preference. Go style in general is to avoid knobs whenever possible, but this might be a case where we need one. I'm not sure what the default would be.

Having HTTP2 clients fail to properly mux concurrent requests is not
something that is acceptable.

Curious: why? We don't expose details of HTTP/1 connection management to net/http users. Why is that optimization detail suddenly so much more import to net/http users when the underlying protocol changes? The net/http API remains unchanged.

@hapnermw

This comment has been minimized.

hapnermw commented Jun 7, 2018

@bradfitz

This comment has been minimized.

Member

bradfitz commented Jun 7, 2018

It actually is an optimization as far as the specs & net/http are concerned. You failed to cite where in the spec it says it's mandatory. (I don't think it is.)

If your particular application depends on such a property, you'll need to arrange to do that yourself. You can use the golang.org/x/net/http2 package directly if needed.

@hapnermw

This comment has been minimized.

hapnermw commented Jun 8, 2018

@hapnermw

This comment has been minimized.

hapnermw commented Jun 12, 2018

@bradfitz

This comment has been minimized.

Member

bradfitz commented Jun 12, 2018

We'll add some way to control this one way or another, but not right now. Go 1.11 is in freeze at the moment (https://github.com/golang/go/wiki/Go-Release-Cycle) so the current priority is fixing Go 1.11 bugs.

@hapnermw

This comment has been minimized.

hapnermw commented Jun 13, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment