Skip to content
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: Transport's automatic http2 too aggressive? #14275

Closed
bradfitz opened this issue Feb 9, 2016 · 6 comments
Closed

net/http: Transport's automatic http2 too aggressive? #14275

bradfitz opened this issue Feb 9, 2016 · 6 comments
Milestone

Comments

@bradfitz
Copy link
Contributor

bradfitz commented Feb 9, 2016

I was just investigating an internal bug report where Go 1.6 broke user's code. The user's code was like:

    transport := &http.Transport{
        TLSClientConfig:     &tls.Config{},
    }
    transport.Dial = func(network, addr string) (net.Conn, error) {
        if isHTTPSProxy(addr) {
            return tls.Dial(network, addr, transport.TLSClientConfig)
        }
        return dialer.Dial(network, addr)
    }

In Go 1.5, that TLSClientConfig was untouched. In Go 1.6, the first call to RoundTrip calls http2.ConfigureTransport, which mutates the Transport.TLSClientConfig.

In this user's case, that meant they sent a TLS NextProto of h2 to the peer, which was accepted, and the peer started speaking http2, but the net/http was trying to make a normal http (not https) request, so http2 wasn't even considered. Then Go's transport was speaking http/1.1 and the server thought it was speaking http2. Fail.

Perhaps we need to rethink how the automatic upgrade to http2 works in Go 1.7.

Maybe as an intermediate conservative step for Go 1.6 we don't auto-enable http2 on Transports where TLSClientConfig is populated, so it doesn't surprise people by being mutated. I was also considering disabling it if Dial or DialTLS are populated, but http.DefaultTransport sets Dial, and I don't want to special-case looking at whether the transport == DefaultTransport.

@bradfitz bradfitz self-assigned this Feb 9, 2016
@bradfitz bradfitz added this to the Go1.7 milestone Feb 9, 2016
@gopherbot
Copy link

CL https://golang.org/cl/19424 mentions this issue.

gopherbot pushed a commit that referenced this issue Feb 9, 2016
For now, don't enable http2 when Transport.TLSConfig != nil.
See background in #14275.

Also don't enable http2 when ExpectContinueTimeout is specified for
now, in case somebody depends on that functionality. (It is not yet
implemented in http2, and was only just added to net/http too in Go
1.6, so nobody would be setting it yet).

Updates #14275
Updates #13851

Change-Id: I192d555f5fb0a567bd89b6ad87175bbdd7891ae3
Reviewed-on: https://go-review.googlesource.com/19424
Reviewed-by: Russ Cox <rsc@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@ailg
Copy link
Contributor

ailg commented Feb 10, 2016

Could we have transport inspect its net.Conn to know whether this one is http/1 or http/2? e.g. if it ends up being a tls.Conn with h2 negociated, then http/2 otherwise http/1?

I implemented this code (a tls.Dial inside transport.Dial) to support HTTPS proxies (#11332) so another option can be to implement this directly in net/http and we could avoid the confusion.

@bradfitz
Copy link
Contributor Author

Could we have transport inspect its net.Conn to know whether this one is http/1 or http/2? e.g. if it ends up being a tls.Conn with h2 negociated, then http/2 otherwise http/1?

It does do that, when it's expecting it. Making it look for that after a plaintext HTTP/1.1 dial would be... weird.

I would rather add support for HTTPS proxies than make such a weird special-case hack.

@rsc
Copy link
Contributor

rsc commented May 18, 2016

What is left for this bug?

@ash2k
Copy link
Contributor

ash2k commented Oct 26, 2016

What about Transport.DialContext field? Should it also be checked in that if?

ash2k added a commit to atlassian/gostatsd that referenced this issue Oct 26, 2016
HTTP2 is disabled if TLSClientConfig/Dial/DialTLS is specified on Transport
See golang/go#14275
ash2k added a commit to atlassian/gostatsd that referenced this issue Oct 26, 2016
HTTP2 is disabled if TLSClientConfig/Dial/DialTLS is specified on Transport
See golang/go#14275
@bradfitz
Copy link
Contributor Author

What about Transport.DialContext field? Should it also be checked in that if?

Comments on closed issues aren't tracked.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants