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

Comments

Projects
None yet
5 participants
@bradfitz
Copy link
Member

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

This comment has been minimized.

Copy link

commented Feb 9, 2016

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

gopherbot pushed a commit that referenced this issue Feb 9, 2016

net/http: be more conservative about enabling http2 on Transports
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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Member Author

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?

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

This comment has been minimized.

Copy link
Contributor

commented May 18, 2016

What is left for this bug?

@ash2k

This comment has been minimized.

Copy link

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

Enable HTTP2
HTTP2 is disabled if TLSClientConfig/Dial/DialTLS is specified on Transport
See golang/go#14275

@ash2k ash2k referenced this issue Oct 26, 2016

Merged

Enable HTTP2 #74

ash2k added a commit to atlassian/gostatsd that referenced this issue Oct 26, 2016

Enable HTTP2 (#74)
HTTP2 is disabled if TLSClientConfig/Dial/DialTLS is specified on Transport
See golang/go#14275
@bradfitz

This comment has been minimized.

Copy link
Member Author

commented Oct 26, 2016

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

Comments on closed issues aren't tracked.

@golang golang locked and limited conversation to collaborators Oct 26, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.