[release-branch.go1.13] net/http: only decrement connection count if we removed a connection
The connection count must only be decremented if the persistent
connection was also removed.
Fixes #36583
Change-Id: I5070717d5d9effec78016005fa4910593500c8cf
Reviewed-on: https://go-review.googlesource.com/c/go/+/202087
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-on: https://go-review.googlesource.com/c/go/+/215177
Run-TryBot: Alexander Rakoczy <alex@golang.org>
Reviewed-by: Alexander Rakoczy <alex@golang.org>
diff --git a/src/net/http/transport.go b/src/net/http/transport.go
index 903e0b5..db8ec4b 100644
--- a/src/net/http/transport.go
+++ b/src/net/http/transport.go
@@ -542,8 +542,9 @@
_, isH2DialError := pconn.alt.(http2erringRoundTripper)
if http2isNoCachedConnError(err) || isH2DialError {
- t.removeIdleConn(pconn)
- t.decConnsPerHost(pconn.cacheKey)
+ if t.removeIdleConn(pconn) {
+ t.decConnsPerHost(pconn.cacheKey)
+ }
}
if !pconn.shouldRetryRequest(req, err) {
// Issue 16465: return underlying net.Conn.Read error from peek,
@@ -966,26 +967,28 @@
}
// removeIdleConn marks pconn as dead.
-func (t *Transport) removeIdleConn(pconn *persistConn) {
+func (t *Transport) removeIdleConn(pconn *persistConn) bool {
t.idleMu.Lock()
defer t.idleMu.Unlock()
- t.removeIdleConnLocked(pconn)
+ return t.removeIdleConnLocked(pconn)
}
// t.idleMu must be held.
-func (t *Transport) removeIdleConnLocked(pconn *persistConn) {
+func (t *Transport) removeIdleConnLocked(pconn *persistConn) bool {
if pconn.idleTimer != nil {
pconn.idleTimer.Stop()
}
t.idleLRU.remove(pconn)
key := pconn.cacheKey
pconns := t.idleConn[key]
+ var removed bool
switch len(pconns) {
case 0:
// Nothing
case 1:
if pconns[0] == pconn {
delete(t.idleConn, key)
+ removed = true
}
default:
for i, v := range pconns {
@@ -996,9 +999,11 @@
// conns at the end.
copy(pconns[i:], pconns[i+1:])
t.idleConn[key] = pconns[:len(pconns)-1]
+ removed = true
break
}
}
+ return removed
}
func (t *Transport) setReqCanceler(r *Request, fn func(error)) {
diff --git a/src/net/http/transport_test.go b/src/net/http/transport_test.go
index d0b12b3..9f31e83 100644
--- a/src/net/http/transport_test.go
+++ b/src/net/http/transport_test.go
@@ -5832,3 +5832,59 @@
t.Errorf("GotConn calls = %v; want %v", got, want)
}
}
+
+// Issue 34941
+// When the client has too many concurrent requests on a single connection,
+// http.http2noCachedConnError is reported on multiple requests. There should
+// only be one decrement regardless of the number of failures.
+func TestTransportDecrementConnWhenIdleConnRemoved(t *testing.T) {
+ defer afterTest(t)
+
+ h := HandlerFunc(func(w ResponseWriter, r *Request) {
+ _, err := w.Write([]byte("foo"))
+ if err != nil {
+ t.Fatalf("Write: %v", err)
+ }
+ })
+
+ ts := httptest.NewUnstartedServer(h)
+ ts.TLS = &tls.Config{NextProtos: []string{"h2"}}
+ ts.StartTLS()
+ defer ts.Close()
+
+ c := ts.Client()
+ tr := c.Transport.(*Transport)
+ tr.MaxConnsPerHost = 1
+ if err := ExportHttp2ConfigureTransport(tr); err != nil {
+ t.Fatalf("ExportHttp2ConfigureTransport: %v", err)
+ }
+
+ errCh := make(chan error, 300)
+ doReq := func() {
+ resp, err := c.Get(ts.URL)
+ if err != nil {
+ errCh <- fmt.Errorf("request failed: %v", err)
+ return
+ }
+ defer resp.Body.Close()
+ _, err = ioutil.ReadAll(resp.Body)
+ if err != nil {
+ errCh <- fmt.Errorf("read body failed: %v", err)
+ }
+ }
+
+ var wg sync.WaitGroup
+ for i := 0; i < 300; i++ {
+ wg.Add(1)
+ go func() {
+ defer wg.Done()
+ doReq()
+ }()
+ }
+ wg.Wait()
+ close(errCh)
+
+ for err := range errCh {
+ t.Errorf("error occurred: %v", err)
+ }
+}
