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: add Transport.MaxConnsPerHost knob, including dial-in-progress conns #13957

Closed
trajber opened this issue Jan 14, 2016 · 17 comments
Closed
Labels
FeatureRequest FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Milestone

Comments

@trajber
Copy link

trajber commented Jan 14, 2016

As far as I know, http2 clients multiplex multiple requests on the same http2 connection.
To test this scenario I created a simple http2 client and server (listed bellow) using go1.6-beta2.
The server handler prints the request.RemoteAddr to ensure that the client is using the same host:port. Until then everything seemed to work properly because all log messages prints the same host and port.
But when I capture the network packets (or just run netstat) I see different client ports in use. The client does not seem multiplexing on the same TCP socket.

I don't know if I'm missing something or if this is a real issue.

Client - http://play.golang.org/p/oKBRCXGbOL
Server - http://play.golang.org/p/AmG6J9qRBv

@bradfitz bradfitz changed the title http2 client: request multiplexing x/net/http2: Transport connection coalescing only half works Jan 14, 2016
@bradfitz bradfitz self-assigned this Jan 14, 2016
@bradfitz bradfitz added this to the Go1.6 milestone Jan 14, 2016
@bradfitz
Copy link
Contributor

Verified.

With client.go:

package main

import (
    "crypto/tls"
    "io/ioutil"
    "log"
    "net/http"
    "sync"
)

func main() {
    tr := &http.Transport{
        TLSClientConfig:    &tls.Config{InsecureSkipVerify: true},
        DisableCompression: false,
    }
    client := &http.Client{Transport: tr}

    var wg sync.WaitGroup
    for i := 0; i < 10; i++ {
        wg.Add(1)
        go func() {
            defer wg.Done()
            resp, err := client.Get("https://localhost:54321/hello")
            if err != nil {
                log.Println(err)
            }
            defer resp.Body.Close()
            slurp, err := ioutil.ReadAll(resp.Body)
            if err != nil {
                log.Fatal(err)
            }
            log.Printf("Got: %q", slurp)
        }()
    }
    wg.Wait()
}

And server:

package main

import (
    "fmt"
    "log"
    "net/http"
    "time"
)

func HelloServer(w http.ResponseWriter, req *http.Request) {
    time.Sleep(1 * time.Second)
    fmt.Fprintf(w, "You are %v, %v", req.RemoteAddr, req.Proto)
}

func main() {
    http.HandleFunc("/hello", HelloServer)
    err := http.ListenAndServeTLS(":54321", "cert.pem", "key.pem", nil)
    if err != nil {
        log.Fatal("ListenAndServe: ", err)
    }
}

I see that http2 does in fact send all requests to the same connection, but it ends up creating those other connections anyway (even though it never uses them):

bradfitz@laptop ~$ GODEBUG=http2debug=2 go run client.go 2>&1 | grep -E 'creating|Framer' 
2016/01/14 11:49:39 http2: Transport creating client conn to [::1]:54321
2016/01/14 11:49:39 http2: Framer 0xc820182b00: wrote SETTINGS len=18, settings: ENABLE_PUSH=0, INITIAL_WINDOW_SIZE=4194304, MAX_HEADER_LIST_SIZE=10485760
2016/01/14 11:49:39 http2: Framer 0xc820182b00: wrote WINDOW_UPDATE len=4 (conn) incr=1073741824
2016/01/14 11:49:39 http2: Transport creating client conn to [::1]:54321
2016/01/14 11:49:39 http2: Framer 0xc820182dc0: wrote SETTINGS len=18, settings: ENABLE_PUSH=0, INITIAL_WINDOW_SIZE=4194304, MAX_HEADER_LIST_SIZE=10485760
2016/01/14 11:49:39 http2: Framer 0xc820182dc0: wrote WINDOW_UPDATE len=4 (conn) incr=1073741824
2016/01/14 11:49:39 http2: Transport creating client conn to [::1]:54321
2016/01/14 11:49:39 http2: Framer 0xc820183080: wrote SETTINGS len=18, settings: ENABLE_PUSH=0, INITIAL_WINDOW_SIZE=4194304, MAX_HEADER_LIST_SIZE=10485760
2016/01/14 11:49:39 http2: Framer 0xc820183080: wrote WINDOW_UPDATE len=4 (conn) incr=1073741824
2016/01/14 11:49:39 http2: Framer 0xc820182b00: read SETTINGS len=18, settings: MAX_FRAME_SIZE=1048576, MAX_CONCURRENT_STREAMS=250, MAX_HEADER_LIST_SIZE=1048896
2016/01/14 11:49:39 http2: Framer 0xc820182b00: wrote SETTINGS flags=ACK len=0
2016/01/14 11:49:39 http2: Framer 0xc820182b00: wrote HEADERS flags=END_STREAM|END_HEADERS stream=1 len=42
2016/01/14 11:49:39 http2: Framer 0xc820182b00: read SETTINGS flags=ACK len=0
2016/01/14 11:49:39 http2: Transport creating client conn to [::1]:54321
2016/01/14 11:49:39 http2: Framer 0xc820183340: wrote SETTINGS len=18, settings: ENABLE_PUSH=0, INITIAL_WINDOW_SIZE=4194304, MAX_HEADER_LIST_SIZE=10485760
2016/01/14 11:49:39 http2: Framer 0xc820183340: wrote WINDOW_UPDATE len=4 (conn) incr=1073741824
2016/01/14 11:49:39 http2: Transport creating client conn to [::1]:54321
2016/01/14 11:49:39 http2: Framer 0xc820183600: wrote SETTINGS len=18, settings: ENABLE_PUSH=0, INITIAL_WINDOW_SIZE=4194304, MAX_HEADER_LIST_SIZE=10485760
2016/01/14 11:49:39 http2: Framer 0xc820183600: wrote WINDOW_UPDATE len=4 (conn) incr=1073741824
2016/01/14 11:49:39 http2: Framer 0xc820183080: read SETTINGS len=18, settings: MAX_FRAME_SIZE=1048576, MAX_CONCURRENT_STREAMS=250, MAX_HEADER_LIST_SIZE=1048896
2016/01/14 11:49:39 http2: Framer 0xc820183080: wrote SETTINGS flags=ACK len=0
2016/01/14 11:49:39 http2: Framer 0xc820182b00: wrote HEADERS flags=END_STREAM|END_HEADERS stream=3 len=6
2016/01/14 11:49:39 http2: Framer 0xc820183080: read SETTINGS flags=ACK len=0
2016/01/14 11:49:39 http2: Framer 0xc820183340: read SETTINGS len=18, settings: MAX_FRAME_SIZE=1048576, MAX_CONCURRENT_STREAMS=250, MAX_HEADER_LIST_SIZE=1048896
2016/01/14 11:49:39 http2: Framer 0xc820183340: wrote SETTINGS flags=ACK len=0
2016/01/14 11:49:39 http2: Framer 0xc820182b00: wrote HEADERS flags=END_STREAM|END_HEADERS stream=5 len=6
2016/01/14 11:49:39 http2: Framer 0xc820183340: read SETTINGS flags=ACK len=0
2016/01/14 11:49:39 http2: Framer 0xc820182dc0: read SETTINGS len=18, settings: MAX_FRAME_SIZE=1048576, MAX_CONCURRENT_STREAMS=250, MAX_HEADER_LIST_SIZE=1048896
2016/01/14 11:49:39 http2: Framer 0xc820182dc0: wrote SETTINGS flags=ACK len=0
2016/01/14 11:49:39 http2: Framer 0xc820182b00: wrote HEADERS flags=END_STREAM|END_HEADERS stream=7 len=6
2016/01/14 11:49:39 http2: Framer 0xc820182dc0: read SETTINGS flags=ACK len=0
2016/01/14 11:49:39 http2: Transport creating client conn to [::1]:54321
2016/01/14 11:49:39 http2: Framer 0xc8201838c0: wrote SETTINGS len=18, settings: ENABLE_PUSH=0, INITIAL_WINDOW_SIZE=4194304, MAX_HEADER_LIST_SIZE=10485760
2016/01/14 11:49:39 http2: Framer 0xc8201838c0: wrote WINDOW_UPDATE len=4 (conn) incr=1073741824
2016/01/14 11:49:39 http2: Framer 0xc820183600: read SETTINGS len=18, settings: MAX_FRAME_SIZE=1048576, MAX_CONCURRENT_STREAMS=250, MAX_HEADER_LIST_SIZE=1048896
2016/01/14 11:49:39 http2: Framer 0xc820183600: wrote SETTINGS flags=ACK len=0
2016/01/14 11:49:39 http2: Framer 0xc820182b00: wrote HEADERS flags=END_STREAM|END_HEADERS stream=9 len=6
2016/01/14 11:49:39 http2: Framer 0xc820183600: read SETTINGS flags=ACK len=0
2016/01/14 11:49:39 http2: Transport creating client conn to [::1]:54321
2016/01/14 11:49:39 http2: Framer 0xc820183b80: wrote SETTINGS len=18, settings: ENABLE_PUSH=0, INITIAL_WINDOW_SIZE=4194304, MAX_HEADER_LIST_SIZE=10485760
2016/01/14 11:49:39 http2: Framer 0xc820183b80: wrote WINDOW_UPDATE len=4 (conn) incr=1073741824
2016/01/14 11:49:39 http2: Framer 0xc8201838c0: read SETTINGS len=18, settings: MAX_FRAME_SIZE=1048576, MAX_CONCURRENT_STREAMS=250, MAX_HEADER_LIST_SIZE=1048896
2016/01/14 11:49:39 http2: Framer 0xc8201838c0: wrote SETTINGS flags=ACK len=0
2016/01/14 11:49:39 http2: Framer 0xc820182b00: wrote HEADERS flags=END_STREAM|END_HEADERS stream=11 len=6
2016/01/14 11:49:39 http2: Framer 0xc8201838c0: read SETTINGS flags=ACK len=0
2016/01/14 11:49:39 http2: Framer 0xc820183b80: read SETTINGS len=18, settings: MAX_FRAME_SIZE=1048576, MAX_CONCURRENT_STREAMS=250, MAX_HEADER_LIST_SIZE=1048896
2016/01/14 11:49:39 http2: Framer 0xc820183b80: wrote SETTINGS flags=ACK len=0
2016/01/14 11:49:39 http2: Transport creating client conn to [::1]:54321
2016/01/14 11:49:39 http2: Framer 0xc8200da8f0: wrote SETTINGS len=18, settings: ENABLE_PUSH=0, INITIAL_WINDOW_SIZE=4194304, MAX_HEADER_LIST_SIZE=10485760
2016/01/14 11:49:39 http2: Framer 0xc8200da8f0: wrote WINDOW_UPDATE len=4 (conn) incr=1073741824
2016/01/14 11:49:39 http2: Transport creating client conn to [::1]:54321
2016/01/14 11:49:39 http2: Framer 0xc820016420: wrote SETTINGS len=18, settings: ENABLE_PUSH=0, INITIAL_WINDOW_SIZE=4194304, MAX_HEADER_LIST_SIZE=10485760
2016/01/14 11:49:39 http2: Framer 0xc820182b00: wrote HEADERS flags=END_STREAM|END_HEADERS stream=13 len=6
2016/01/14 11:49:39 http2: Framer 0xc820016420: wrote WINDOW_UPDATE len=4 (conn) incr=1073741824
2016/01/14 11:49:39 http2: Framer 0xc820183b80: read SETTINGS flags=ACK len=0
2016/01/14 11:49:39 http2: Framer 0xc820016420: read SETTINGS len=18, settings: MAX_FRAME_SIZE=1048576, MAX_CONCURRENT_STREAMS=250, MAX_HEADER_LIST_SIZE=1048896
2016/01/14 11:49:39 http2: Framer 0xc820016420: wrote SETTINGS flags=ACK len=0
2016/01/14 11:49:39 http2: Framer 0xc820182b00: wrote HEADERS flags=END_STREAM|END_HEADERS stream=15 len=6
2016/01/14 11:49:39 http2: Framer 0xc8200da8f0: read SETTINGS len=18, settings: MAX_FRAME_SIZE=1048576, MAX_CONCURRENT_STREAMS=250, MAX_HEADER_LIST_SIZE=1048896
2016/01/14 11:49:39 http2: Framer 0xc820016420: read SETTINGS flags=ACK len=0
2016/01/14 11:49:39 http2: Framer 0xc8200da8f0: wrote SETTINGS flags=ACK len=0
2016/01/14 11:49:39 http2: Transport creating client conn to [::1]:54321
2016/01/14 11:49:39 http2: Framer 0xc820201760: wrote SETTINGS len=18, settings: ENABLE_PUSH=0, INITIAL_WINDOW_SIZE=4194304, MAX_HEADER_LIST_SIZE=10485760
2016/01/14 11:49:39 http2: Framer 0xc820201760: wrote WINDOW_UPDATE len=4 (conn) incr=1073741824
2016/01/14 11:49:39 http2: Framer 0xc8200da8f0: read SETTINGS flags=ACK len=0
2016/01/14 11:49:39 http2: Framer 0xc820182b00: wrote HEADERS flags=END_STREAM|END_HEADERS stream=17 len=6
2016/01/14 11:49:39 http2: Framer 0xc820201760: read SETTINGS len=18, settings: MAX_FRAME_SIZE=1048576, MAX_CONCURRENT_STREAMS=250, MAX_HEADER_LIST_SIZE=1048896
2016/01/14 11:49:39 http2: Framer 0xc820201760: wrote SETTINGS flags=ACK len=0
2016/01/14 11:49:39 http2: Framer 0xc820201760: read SETTINGS flags=ACK len=0
2016/01/14 11:49:39 http2: Framer 0xc820182b00: wrote HEADERS flags=END_STREAM|END_HEADERS stream=19 len=6
2016/01/14 11:49:40 http2: Framer 0xc820182b00: read HEADERS flags=END_HEADERS stream=1 len=49
2016/01/14 11:49:40 http2: Framer 0xc820182b00: read HEADERS flags=END_HEADERS stream=7 len=4
2016/01/14 11:49:40 http2: Framer 0xc820182b00: read HEADERS flags=END_HEADERS stream=5 len=4
2016/01/14 11:49:40 http2: Framer 0xc820182b00: read HEADERS flags=END_HEADERS stream=13 len=4
2016/01/14 11:49:40 http2: Framer 0xc820182b00: read HEADERS flags=END_HEADERS stream=11 len=4
2016/01/14 11:49:40 http2: Framer 0xc820182b00: read HEADERS flags=END_HEADERS stream=15 len=4
2016/01/14 11:49:40 http2: Framer 0xc820182b00: read HEADERS flags=END_HEADERS stream=19 len=4
2016/01/14 11:49:40 http2: Framer 0xc820182b00: read HEADERS flags=END_HEADERS stream=9 len=4
2016/01/14 11:49:40 http2: Framer 0xc820182b00: read HEADERS flags=END_HEADERS stream=3 len=4
2016/01/14 11:49:40 http2: Framer 0xc820182b00: read HEADERS flags=END_HEADERS stream=17 len=4
2016/01/14 11:49:40 http2: Framer 0xc820182b00: read DATA flags=END_STREAM stream=1 len=29 data="You are [::1]:55709, HTTP/2.0"
2016/01/14 11:49:40 http2: Framer 0xc820182b00: read DATA flags=END_STREAM stream=11 len=29 data="You are [::1]:55709, HTTP/2.0"
2016/01/14 11:49:40 http2: Framer 0xc820182b00: read DATA flags=END_STREAM stream=5 len=29 data="You are [::1]:55709, HTTP/2.0"
2016/01/14 11:49:40 http2: Framer 0xc820182b00: read DATA flags=END_STREAM stream=17 len=29 data="You are [::1]:55709, HTTP/2.0"
2016/01/14 11:49:40 http2: Framer 0xc820182b00: read DATA flags=END_STREAM stream=7 len=29 data="You are [::1]:55709, HTTP/2.0"
2016/01/14 11:49:40 http2: Framer 0xc820182b00: read DATA flags=END_STREAM stream=13 len=29 data="You are [::1]:55709, HTTP/2.0"
2016/01/14 11:49:40 http2: Framer 0xc820182b00: read DATA flags=END_STREAM stream=19 len=29 data="You are [::1]:55709, HTTP/2.0"
2016/01/14 11:49:40 http2: Framer 0xc820182b00: read DATA flags=END_STREAM stream=3 len=29 data="You are [::1]:55709, HTTP/2.0"
2016/01/14 11:49:40 http2: Framer 0xc820182b00: read DATA flags=END_STREAM stream=9 len=29 data="You are [::1]:55709, HTTP/2.0"
2016/01/14 11:49:40 http2: Framer 0xc820182b00: read DATA flags=END_STREAM stream=15 len=29 data="You are [::1]:55709, HTTP/2.0"

@bradfitz
Copy link
Contributor

Oh, right, I remember the issue here.

The fundamental problem is that at the time the http.Transport gets a RoundTrip call, it doesn't know which protocol the peer supports.

If two goroutines ask a URL at the same time, there are two possible best answers, depending on which protocol the peer supports (which we don't know)

If the peer only supports HTTP/1, the best strategy is to start two TCP connects immediately, so both requests can be sent once both TCP setups are complete.

If the peer supports HTTP/2, the best strategy (at least to minimize resources), is to only have 1 TCP connect in flight, wait for it to finish, and then send both requests on the same connection.

Unfortunately, we don't know. And we don't have a place to keep state to learn over time, either (not that we'd necessarily want to).

Various solutions:

  1. hard-code popular site hostnames known to support http2. Kinda gross, wastes space, maintenance burden, doesn't scale.
  2. always bound max connects at 1. penalizes HTTP/1, but is best for http2.
  3. always bound max connects at N. perhaps 2 or 3. Good compromise?
  4. do 3 + also learn over time, at least storing the state only in memory? But once you're connected, you tend to stay connected. and you have to bound memory usage somehow. which hosts do you remember support http2? this is too big

We should at least shut down those idle http2 connections once they're connected, though. Right now we just sit on them and they stay idle, running and replying to pings, etc. That's a waste.

@bradfitz bradfitz changed the title x/net/http2: Transport connection coalescing only half works net/http: limit Transport per-host connections in-flight, close unwanted ones Jan 14, 2016
@bradfitz
Copy link
Contributor

(re-titled the bug, as this has nothing to do with x/net/http2 which does properly merge connections. The problem is solely in the integration with the http1 Transport)

@trajber
Copy link
Author

trajber commented Jan 14, 2016

In some situations we do know if the peer supports http2.
What if http.Transport had an attribute to always bound max conns at 1 (your second solution)? It will be used when we know that the server supports http2.
In other situations the http.Transport tries to discover what the peer supports.

@bradfitz
Copy link
Contributor

In some situations we do know if the peer supports http2.

If you know your peer supports http2, then use https://godoc.org/golang.org/x/net/http2#Transport directly.

What if http.Transport had an attribute to always bound max conns at 1 (your second solution)? It will be used when we know that the server supports http2.

We're not adding any new API surface (no new knobs) this late in the release cycle.

We're probably not even going to bound the number of TCP connects we do for Go 1.6. I have a change to shut down the unneeded connections at least.

@bradfitz
Copy link
Contributor

Sent https://go-review.googlesource.com/#/c/18675/ for closing unneeded connections and https://go-review.googlesource.com/#/c/18676/ to bring it into std, with tests.

@gopherbot
Copy link

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

@gopherbot
Copy link

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

gopherbot pushed a commit to golang/net that referenced this issue Jan 15, 2016
If a user starts two HTTP requests when no http2 connection is
available, both end up creating new TCP connections, since the
server's protocol (h1 or h2) isn't yet known. Once it turns out that
the server supports h2, one of the connections is useless. Previously
we kept upgrading both TLS connections to h2 (SETTINGS frame exchange,
etc).  Now the unnecessary connections are closed instead, before the
h2 preface/SETTINGS.

Tests in the standard library (where it's easier to test), in the
commit which updates h2_bundle.go with this change.

Updates golang/go#13957

Change-Id: I5af177e6ea755d572b551cc0b0de9da865ef4ae7
Reviewed-on: https://go-review.googlesource.com/18675
Reviewed-by: Andrew Gerrand <adg@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
gopherbot pushed a commit that referenced this issue Jan 15, 2016
If a user starts two HTTP requests when no http2 connection is
available, both end up creating new TCP connections, since the
server's protocol (h1 or h2) isn't yet known. Once it turns out that
the server supports h2, one of the connections is useless. Previously
we kept upgrading both TLS connections to h2 (SETTINGS frame exchange,
etc).  Now the unnecessary connections are closed instead, before the
h2 preface/SETTINGS.

Updates x/net/http2 to git rev a8e212f3d for https://golang.org/cl/18675

This CL contains the tests for https://golang.org/cl/18675

Semi-related change noticed while writing the tests: now that we have
TLSNextProto in Go 1.6, which consults the TLS
ConnectionState.NegotiatedProtocol, we have to gurantee that the TLS
handshake has been done before we look at the ConnectionState. So add
that check after the DialTLS hook. (we never documented that users
have to call Handshake, so do it for them, now that it matters)

Updates #13957

Change-Id: I9a70e9d1282fe937ea654d9b1269c984c4e366c0
Reviewed-on: https://go-review.googlesource.com/18676
Reviewed-by: Andrew Gerrand <adg@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@bradfitz
Copy link
Contributor

Okay, those two CLs are probably enough for Go 1.6. If it's easy, I might limit the max pending dials per host to 5 or 6, like browsers, but it's not a huge deal if it slips. It's been how it is for ages.

@trajber
Copy link
Author

trajber commented Feb 22, 2016

This issue has been resolved? I noticed that the CLs was merged into Go1.6 final code but when I try to send a 10k HTTP/2 concurrent requests the client still opening too many connections.
In some situations it exits with the error "socket: too many open files".

@bradfitz
Copy link
Contributor

It has not been resolved. Some CLs which help were merged.

@gopherbot
Copy link

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

@bradfitz
Copy link
Contributor

Per discussion with @jbardin on https://golang.org/cl/20485, we've decided to not limit in-flight dials (which is a weird knob) and instead just have a MaxConnsPerHost knob, next to the existing MaxIdleConnsPerHost knob.

Repurposing this bug.

@bradfitz bradfitz removed their assignment May 19, 2016
@bradfitz bradfitz changed the title net/http: limit Transport per-host connections in-flight, close unwanted ones net/http: add Transport.MaxConnsPerHost knob, including dial-in-progress conns May 19, 2016
gopherbot pushed a commit that referenced this issue Aug 8, 2016
…y limit

The Go HTTP/1 client will make as many new TCP connections as the user requests.

The HTTP/2 client tried to have that behavior, but the policy of
whether a connection is re-usable didn't take into account the extra 1
stream counting against SETTINGS_MAX_CONCURRENT_STREAMS so in practice
users were getting errors.

For example, if the server's advertised max concurrent streams is 100
and 200 concurrrent Go HTTP requests ask for a connection at once, all
200 will think they can reuse that TCP connection, but then 100 will
fail later when the number of concurrent streams exceeds 100.

Instead, recognize the "no cached connections" error value in the
shouldRetryRequest method, so those 100 will retry a new connection.

This is the conservative fix for Go 1.7 so users don't get errors, and
to match the HTTP/1 behavior. Issues #13957 and #13774 are the more
involved bugs for Go 1.8.

Updates #16582
Updates #13957

Change-Id: I1f15a7ce60c07a4baebca87675836d6fe03993e8
Reviewed-on: https://go-review.googlesource.com/25580
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Chris Broadfoot <cbro@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
madeye pushed a commit to shadowsocks/go that referenced this issue Aug 10, 2016
…y limit

The Go HTTP/1 client will make as many new TCP connections as the user requests.

The HTTP/2 client tried to have that behavior, but the policy of
whether a connection is re-usable didn't take into account the extra 1
stream counting against SETTINGS_MAX_CONCURRENT_STREAMS so in practice
users were getting errors.

For example, if the server's advertised max concurrent streams is 100
and 200 concurrrent Go HTTP requests ask for a connection at once, all
200 will think they can reuse that TCP connection, but then 100 will
fail later when the number of concurrent streams exceeds 100.

Instead, recognize the "no cached connections" error value in the
shouldRetryRequest method, so those 100 will retry a new connection.

This is the conservative fix for Go 1.7 so users don't get errors, and
to match the HTTP/1 behavior. Issues golang#13957 and golang#13774 are the more
involved bugs for Go 1.8.

Updates golang#16582
Updates golang#13957

Change-Id: I1f15a7ce60c07a4baebca87675836d6fe03993e8
Reviewed-on: https://go-review.googlesource.com/25580
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Chris Broadfoot <cbro@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
@quentinmit quentinmit added the NeedsFix The path to resolution is known, but the work has not been done. label Oct 7, 2016
@bradfitz
Copy link
Contributor

bradfitz commented Nov 1, 2016

Didn't happen for Go 1.8.

@gopherbot
Copy link

Change https://golang.org/cl/71272 mentions this issue: net/http: add MaxConnsPerHost knob

@bradfitz
Copy link
Contributor

bradfitz commented Jan 4, 2018

Too late for new API for Go 1.10, but I've marked this as a release-blocker for Go 1.11 because it has a pending CL (that needs more review).

c3mb0 pushed a commit to c3mb0/net that referenced this issue Apr 2, 2018
If a user starts two HTTP requests when no http2 connection is
available, both end up creating new TCP connections, since the
server's protocol (h1 or h2) isn't yet known. Once it turns out that
the server supports h2, one of the connections is useless. Previously
we kept upgrading both TLS connections to h2 (SETTINGS frame exchange,
etc).  Now the unnecessary connections are closed instead, before the
h2 preface/SETTINGS.

Tests in the standard library (where it's easier to test), in the
commit which updates h2_bundle.go with this change.

Updates golang/go#13957

Change-Id: I5af177e6ea755d572b551cc0b0de9da865ef4ae7
Reviewed-on: https://go-review.googlesource.com/18675
Reviewed-by: Andrew Gerrand <adg@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@gopherbot
Copy link

Change https://golang.org/cl/151857 mentions this issue: http2: revert Transport's strict interpretation of MAX_CONCURRENT_STREAMS

gopherbot pushed a commit to golang/net that referenced this issue Dec 1, 2018
…EAMS

And add the http2.Transport.StrictMaxConcurrentStreams bool knob to
behavior being reverted.

In CL 53250 for golang/go#13774 (for Go 1.10) we changed the HTTP/2
Transport's policy such that a server's advertisement of a
MAX_CONCURRENT_STREAMS value meant that it was a maximum for the
entire process, instead of just a single connection.

We thought that was a reasonable interpretation of the spec and
provided nice safety against slamming a server from a bunch of
goroutines doing concurrent requests, but it's been largely
unpopular (see golang/go#27044). It's also different behavior from
HTTP/1 and because you're usually not sure which protocol version
you're going to get, you need to limit your outbound HTTP requests
anyway in case you're hitting an HTTP/1 server.

And nowadays we have the Go 1.11 Transport.MaxConnsPerHost knob too
(CL 71272 for golang/go#13957). It doesn't yet work for HTTP/2, but it
will in either Go 1.12 or Go 1.13 (golang/go#27753)

After this is bundled into net/http's, the default HTTP client will
have this knob set false, restoring the old Go 1.9 behavior where new
TCP connections are created as necessary. Users wanting the strict
behavior and import golang.org/x/net/http2 themselves and make a
Transport with StrictMaxConcurrentStreams set to true. Or they can set
Transport.MaxConnsPerHost, once that works for HTTP/2.

Updates golang/go#27044 (fixes after bundle into std)

Change-Id: I4efdad7698feaf674ee8e01032d2dfa5c2f8a3a8
Reviewed-on: https://go-review.googlesource.com/c/151857
Reviewed-by: Andrew Bonventre <andybons@golang.org>
froodian pushed a commit to Appboy/net that referenced this issue Jan 7, 2019
…EAMS

And add the http2.Transport.StrictMaxConcurrentStreams bool knob to
behavior being reverted.

In CL 53250 for golang/go#13774 (for Go 1.10) we changed the HTTP/2
Transport's policy such that a server's advertisement of a
MAX_CONCURRENT_STREAMS value meant that it was a maximum for the
entire process, instead of just a single connection.

We thought that was a reasonable interpretation of the spec and
provided nice safety against slamming a server from a bunch of
goroutines doing concurrent requests, but it's been largely
unpopular (see golang/go#27044). It's also different behavior from
HTTP/1 and because you're usually not sure which protocol version
you're going to get, you need to limit your outbound HTTP requests
anyway in case you're hitting an HTTP/1 server.

And nowadays we have the Go 1.11 Transport.MaxConnsPerHost knob too
(CL 71272 for golang/go#13957). It doesn't yet work for HTTP/2, but it
will in either Go 1.12 or Go 1.13 (golang/go#27753)

After this is bundled into net/http's, the default HTTP client will
have this knob set false, restoring the old Go 1.9 behavior where new
TCP connections are created as necessary. Users wanting the strict
behavior and import golang.org/x/net/http2 themselves and make a
Transport with StrictMaxConcurrentStreams set to true. Or they can set
Transport.MaxConnsPerHost, once that works for HTTP/2.

Updates golang/go#27044 (fixes after bundle into std)

Change-Id: I4efdad7698feaf674ee8e01032d2dfa5c2f8a3a8
Reviewed-on: https://go-review.googlesource.com/c/151857
Reviewed-by: Andrew Bonventre <andybons@golang.org>
froodian added a commit to Appboy/net that referenced this issue Jan 9, 2019
…EAMS (#1)

And add the http2.Transport.StrictMaxConcurrentStreams bool knob to
behavior being reverted.

In CL 53250 for golang/go#13774 (for Go 1.10) we changed the HTTP/2
Transport's policy such that a server's advertisement of a
MAX_CONCURRENT_STREAMS value meant that it was a maximum for the
entire process, instead of just a single connection.

We thought that was a reasonable interpretation of the spec and
provided nice safety against slamming a server from a bunch of
goroutines doing concurrent requests, but it's been largely
unpopular (see golang/go#27044). It's also different behavior from
HTTP/1 and because you're usually not sure which protocol version
you're going to get, you need to limit your outbound HTTP requests
anyway in case you're hitting an HTTP/1 server.

And nowadays we have the Go 1.11 Transport.MaxConnsPerHost knob too
(CL 71272 for golang/go#13957). It doesn't yet work for HTTP/2, but it
will in either Go 1.12 or Go 1.13 (golang/go#27753)

After this is bundled into net/http's, the default HTTP client will
have this knob set false, restoring the old Go 1.9 behavior where new
TCP connections are created as necessary. Users wanting the strict
behavior and import golang.org/x/net/http2 themselves and make a
Transport with StrictMaxConcurrentStreams set to true. Or they can set
Transport.MaxConnsPerHost, once that works for HTTP/2.

Updates golang/go#27044 (fixes after bundle into std)

Change-Id: I4efdad7698feaf674ee8e01032d2dfa5c2f8a3a8
Reviewed-on: https://go-review.googlesource.com/c/151857
Reviewed-by: Andrew Bonventre <andybons@golang.org>
@golang golang locked and limited conversation to collaborators Nov 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FeatureRequest FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Projects
None yet
Development

No branches or pull requests

4 participants