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: Go 1.6 http.Client doesn't support http2 by default #14391

Closed
kron4eg opened this Issue Feb 18, 2016 · 14 comments

Comments

Projects
None yet
10 participants
@kron4eg

kron4eg commented Feb 18, 2016

go version go1.6 darwin/amd64

It's said in documentation that http2 is enabled by default in release 1.6 for server and client, but client refuses to communicate http2.

package main

import (
    "fmt"
    "io/ioutil"
    "log"
    "net/http"
)

func main() {
    response, err := http.Get("https://http2.golang.org/reqinfo")
    if err != nil {
        log.Fatal(err)
    }
    fmt.Printf("is HTTP2: %v (%s)\n\n", response.ProtoAtLeast(2, 0), response.Proto)
    body, err := ioutil.ReadAll(response.Body)
    if err != nil {
        log.Fatal(err)
    }
    fmt.Println(string(body))
}

returns

is HTTP2: false (HTTP/1.1)

Method: GET
Protocol: HTTP/1.1
Host: http2.golang.org
RemoteAddr: REMOVED:33882
RequestURI: "/reqinfo"
URL: &url.URL{Scheme:"", Opaque:"", User:(*url.Userinfo)(nil), Host:"", Path:"/reqinfo", RawPath:"", RawQuery:"", Fragment:""}
Body.ContentLength: 0 (-1 means unknown)
Close: false (relevant for HTTP/1 only)
TLS: &tls.ConnectionState{Version:0x303, HandshakeComplete:true, DidResume:false, CipherSuite:0xc02f, NegotiatedProtocol:"", NegotiatedProtocolIsMutual:true, ServerName:"http2.golang.org", PeerCertificates:[]*x509.Certificate(nil), VerifiedChains:[][]*x509.Certificate(nil), SignedCertificateTimestamps:[][]uint8(nil), OCSPResponse:[]uint8(nil), TLSUnique:[]uint8{0xcf, 0x67, 0x6b, 0xfb, 0x3c, 0xc2, 0xc6, 0xc2, 0xb9, 0xfb, 0x1b, 0x24}}

Headers:
Accept-Encoding: gzip
User-Agent: Go-http-client/1.1

@ianlancetaylor ianlancetaylor changed the title from x/net/http2: In Go 1.6 http.Client doesn't support http2 by default to net/http: Go 1.6 http.Client doesn't support http2 by default Feb 18, 2016

@ianlancetaylor ianlancetaylor added this to the Go1.6.1 milestone Feb 18, 2016

@bradfitz

This comment has been minimized.

Show comment
Hide comment
@bradfitz

bradfitz Feb 18, 2016

Member

Well, crap. :-(

I broke it at the last second in 79d9f48 with the ExpectContinueTimeout because the http.DefaultTransport defines an ExpectContinueTimeout.

And the unit test to check for automatic http2 upgrade doesn't use DefaultTransport:

func TestTransportAutomaticHTTP2(t *testing.T) {
        testTransportAutoHTTP(t, &Transport{}, true)
}

func TestTransportAutomaticHTTP2_TLSNextProto(t *testing.T) {
        testTransportAutoHTTP(t, &Transport{
                TLSNextProto: make(map[string]func(string, *tls.Conn) RoundTripper),
        }, false)
}

func TestTransportAutomaticHTTP2_TLSConfig(t *testing.T) {
        testTransportAutoHTTP(t, &Transport{
                TLSClientConfig: new(tls.Config),
        }, false)
}

func TestTransportAutomaticHTTP2_ExpectContinueTimeout(t *testing.T) {
        testTransportAutoHTTP(t, &Transport{
                ExpectContinueTimeout: 1 * time.Second,
        }, false)
}

I think DefaultTransport should have it enabled, regardless of its ExpectContinueTimeout.

Member

bradfitz commented Feb 18, 2016

Well, crap. :-(

I broke it at the last second in 79d9f48 with the ExpectContinueTimeout because the http.DefaultTransport defines an ExpectContinueTimeout.

And the unit test to check for automatic http2 upgrade doesn't use DefaultTransport:

func TestTransportAutomaticHTTP2(t *testing.T) {
        testTransportAutoHTTP(t, &Transport{}, true)
}

func TestTransportAutomaticHTTP2_TLSNextProto(t *testing.T) {
        testTransportAutoHTTP(t, &Transport{
                TLSNextProto: make(map[string]func(string, *tls.Conn) RoundTripper),
        }, false)
}

func TestTransportAutomaticHTTP2_TLSConfig(t *testing.T) {
        testTransportAutoHTTP(t, &Transport{
                TLSClientConfig: new(tls.Config),
        }, false)
}

func TestTransportAutomaticHTTP2_ExpectContinueTimeout(t *testing.T) {
        testTransportAutoHTTP(t, &Transport{
                ExpectContinueTimeout: 1 * time.Second,
        }, false)
}

I think DefaultTransport should have it enabled, regardless of its ExpectContinueTimeout.

@FiloSottile

This comment has been minimized.

Show comment
Hide comment
@FiloSottile

FiloSottile Feb 20, 2016

Member

I'm not 100% convinced by the fix. I would not expect a copy of DefaultTransport to behave differently from DefaultTransport. It's potentially really surprising.

Member

FiloSottile commented Feb 20, 2016

I'm not 100% convinced by the fix. I would not expect a copy of DefaultTransport to behave differently from DefaultTransport. It's potentially really surprising.

@bradfitz

This comment has been minimized.

Show comment
Hide comment
@bradfitz

bradfitz Feb 20, 2016

Member

Got an alternative proposal?

Note that ExpectContinueTimeout was only added in Go 1.6 also, so nobody will have a copy of this DefaultTransport in their code already.

Member

bradfitz commented Feb 20, 2016

Got an alternative proposal?

Note that ExpectContinueTimeout was only added in Go 1.6 also, so nobody will have a copy of this DefaultTransport in their code already.

@FiloSottile

This comment has been minimized.

Show comment
Hide comment
@FiloSottile

FiloSottile Feb 20, 2016

Member

Not really, I'm doing some serious peanut gallery here.

However, I did copy paste the DefaultTransport before, or I could see why someone would type assert it and dereference it to get a "future-proof" copy of the DefaultTransport. Those users would unexpectedly get no HTTP/2. Even worse, I can see a small change moving some code from DefaultTransport to a copy of DefaultTransport passing code review as "sure that does not change anything", and instead degrading service. And I see no good way to figure out why two identical Transport behave differently.

Maybe just plainly documenting that ExpectContinueTimeout does not apply to HTTP/2 connections and always ignoring it is more clean and simple. Also considering that ExpectContinueTimeout is new and no one is relying on it yet.

Member

FiloSottile commented Feb 20, 2016

Not really, I'm doing some serious peanut gallery here.

However, I did copy paste the DefaultTransport before, or I could see why someone would type assert it and dereference it to get a "future-proof" copy of the DefaultTransport. Those users would unexpectedly get no HTTP/2. Even worse, I can see a small change moving some code from DefaultTransport to a copy of DefaultTransport passing code review as "sure that does not change anything", and instead degrading service. And I see no good way to figure out why two identical Transport behave differently.

Maybe just plainly documenting that ExpectContinueTimeout does not apply to HTTP/2 connections and always ignoring it is more clean and simple. Also considering that ExpectContinueTimeout is new and no one is relying on it yet.

@bradfitz

This comment has been minimized.

Show comment
Hide comment
@bradfitz

bradfitz Feb 20, 2016

Member

I agree that it's weird that two identical DefaultTransports would behave differently. I mentioned the same concern in some earlier review. (hotel wifi too terrible for me to find it at the moment)

Yeah, perhaps some documentation on ExpectContinueTimeout would be better for now. I do intend to make it work for Go 1.7.

@rsc, opinions?

Member

bradfitz commented Feb 20, 2016

I agree that it's weird that two identical DefaultTransports would behave differently. I mentioned the same concern in some earlier review. (hotel wifi too terrible for me to find it at the moment)

Yeah, perhaps some documentation on ExpectContinueTimeout would be better for now. I do intend to make it work for Go 1.7.

@rsc, opinions?

@gripedthumbtacks

This comment has been minimized.

Show comment
Hide comment
@gripedthumbtacks

gripedthumbtacks Mar 5, 2016

I am told it is confirmed that go v1.6 initial release does not work with http2, by default, but that it is currently planned for v1.6.1. Looking forward to it :)

gripedthumbtacks commented Mar 5, 2016

I am told it is confirmed that go v1.6 initial release does not work with http2, by default, but that it is currently planned for v1.6.1. Looking forward to it :)

@elithrar

This comment has been minimized.

Show comment
Hide comment
@elithrar

elithrar Mar 5, 2016

@kristian-hermansen To clarify: Go 1.6 supports ("works with") HTTP/2, but requires you to modify the Transport. You don't have to wait until Go 1.6.1.

elithrar commented Mar 5, 2016

@kristian-hermansen To clarify: Go 1.6 supports ("works with") HTTP/2, but requires you to modify the Transport. You don't have to wait until Go 1.6.1.

@gripedthumbtacks

This comment has been minimized.

Show comment
Hide comment
@gripedthumbtacks

gripedthumbtacks Mar 6, 2016

Right, but not by default, which I guess was what brought me here googling. In a golang support chat, someone told me to wait until v1.6.1 or I might have to change the code again. Is that true, or will modifying the Transport work even after v1.6.1 is out? And if so, do you have some example code that would be forward compatible (since we don't know what changes are planned)? Thanks much! :)

gripedthumbtacks commented Mar 6, 2016

Right, but not by default, which I guess was what brought me here googling. In a golang support chat, someone told me to wait until v1.6.1 or I might have to change the code again. Is that true, or will modifying the Transport work even after v1.6.1 is out? And if so, do you have some example code that would be forward compatible (since we don't know what changes are planned)? Thanks much! :)

@mdevan

This comment has been minimized.

Show comment
Hide comment
@mdevan

mdevan Mar 28, 2016

As a workaround, for Go1.6, do this before you use the client:

if tr, ok := http.DefaultTransport.(*http.Transport); ok {
    tr.ExpectContinueTimeout = 0
}

IMHO, the HTTP/2 code should just be ignoring ExpectContinueTimeout if it is not relevant for HTTP/2, and the documentation of ExpectContinueTimeout should say that it's applicable only for HTTP/1.x.

mdevan commented Mar 28, 2016

As a workaround, for Go1.6, do this before you use the client:

if tr, ok := http.DefaultTransport.(*http.Transport); ok {
    tr.ExpectContinueTimeout = 0
}

IMHO, the HTTP/2 code should just be ignoring ExpectContinueTimeout if it is not relevant for HTTP/2, and the documentation of ExpectContinueTimeout should say that it's applicable only for HTTP/1.x.

@FiloSottile

This comment has been minimized.

Show comment
Hide comment
@FiloSottile

FiloSottile Mar 28, 2016

Member

This weekend I found myself copying the DefaultTransport twice, and I thought about this again. One very common example is defining a TLSClientConfig (without losing the timeouts).

Not sure if I like typeassert-dereference-copy or copy-paste more, but both will break HTTP/2 if DefaultTransport is special-cased.

@bradfitz @rsc, did any discussion happen on simply ignoring ExpectContinueTimeout to avoid magic behavior?

Member

FiloSottile commented Mar 28, 2016

This weekend I found myself copying the DefaultTransport twice, and I thought about this again. One very common example is defining a TLSClientConfig (without losing the timeouts).

Not sure if I like typeassert-dereference-copy or copy-paste more, but both will break HTTP/2 if DefaultTransport is special-cased.

@bradfitz @rsc, did any discussion happen on simply ignoring ExpectContinueTimeout to avoid magic behavior?

@adg adg added Release-OK and removed Release-OK labels Apr 7, 2016

@adg

This comment has been minimized.

Show comment
Hide comment
@adg

adg Apr 7, 2016

Contributor

I'd like this to go into 1.6.1 but I am concerned on springing a major change on unsuspecting users. They thought they were getting http2 in 1.6, but then didn't, and if we turn it on now I worry things will break. Was http2 working in the release candidates? That would go some way to assuaging my fears.

Contributor

adg commented Apr 7, 2016

I'd like this to go into 1.6.1 but I am concerned on springing a major change on unsuspecting users. They thought they were getting http2 in 1.6, but then didn't, and if we turn it on now I worry things will break. Was http2 working in the release candidates? That would go some way to assuaging my fears.

@bradfitz bradfitz modified the milestones: Go1.6.2, Go1.6.1 Apr 7, 2016

@bradfitz

This comment has been minimized.

Show comment
Hide comment
@bradfitz

bradfitz Apr 7, 2016

Member

@adg, 1.6.1 is for security stuff only. This could go into Go 1.6.2, which might happen at about the same time. Yes, this worked in rc releases. I worried about the surprise change, but there are already at least two surprise elements to this as-is:

  1. new(Transport) and http.Get can differ.
  2. new(Transport) to foo.com can do one thing today, and a different thing tomorrow, regardless of your Go version, just because foo.com added HTTP/2 support. So people were already making major codepath changes somewhat outside of their control. (but they could force-override it to HTTP/1)
Member

bradfitz commented Apr 7, 2016

@adg, 1.6.1 is for security stuff only. This could go into Go 1.6.2, which might happen at about the same time. Yes, this worked in rc releases. I worried about the surprise change, but there are already at least two surprise elements to this as-is:

  1. new(Transport) and http.Get can differ.
  2. new(Transport) to foo.com can do one thing today, and a different thing tomorrow, regardless of your Go version, just because foo.com added HTTP/2 support. So people were already making major codepath changes somewhat outside of their control. (but they could force-override it to HTTP/1)
@rsc

This comment has been minimized.

Show comment
Hide comment
@rsc

rsc Apr 7, 2016

Contributor

Also, if Go 1.6.2's correct use of HTTP/2 in the default client breaks something, they've already got the environment variable to turn it off. We should make that clear in the point release notes.

Contributor

rsc commented Apr 7, 2016

Also, if Go 1.6.2's correct use of HTTP/2 in the default client breaks something, they've already got the environment variable to turn it off. We should make that clear in the point release notes.

@rsc rsc added Release-OK and removed Release-Maybe labels Apr 7, 2016

@gopherbot

This comment has been minimized.

Show comment
Hide comment
@gopherbot

gopherbot commented Apr 14, 2016

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

gopherbot pushed a commit that referenced this issue Apr 14, 2016

net/http: fix bug where http2 wasn't enabled on DefaultTransport
I had accidentally disabled a headline feature at the last second. :(

Fixes #14391

Change-Id: I1992c9b801072b7538b95c55242be174075ff932
Reviewed-on: https://go-review.googlesource.com/19672
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-on: https://go-review.googlesource.com/22035
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>

@adg adg added the Release-Applied label Apr 14, 2016

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