net/http: add Transport.MaxConnsPerHost knob, including dial-in-progress conns #13957
Labels
FeatureRequest
Issues asking for a new feature that does not need a proposal.
FrozenDueToAge
help wanted
NeedsFix
The path to resolution is known, but the work has not been done.
release-blocker
Milestone
Comments
|
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): |
|
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:
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. |
|
(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) |
|
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.
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. |
|
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. |
|
CL https://golang.org/cl/18676 mentions this issue. |
|
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>
|
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. |
|
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. |
|
It has not been resolved. Some CLs which help were merged. |
|
CL https://golang.org/cl/20485 mentions this issue. |
|
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 Repurposing this bug. |
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>
|
Didn't happen for Go 1.8. |
|
Change https://golang.org/cl/71272 mentions this issue: |
|
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>
|
Change https://golang.org/cl/151857 mentions this issue: |
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>
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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
The text was updated successfully, but these errors were encountered: