[release-branch.go1.13] net/http: fix HTTP/2 idle pool tracing

CL 140357 caused HTTP/2 connections to be put in the idle pool, but
failed to properly guard the trace.GotConn call in getConn. dialConn
returns a minimal persistConn with conn == nil for HTTP/2 connections.
This persistConn was then returned from queueForIdleConn and caused the
httptrace.GotConnInfo passed into GotConn to have a nil Conn field.

HTTP/2 connections call GotConn themselves so leave it for HTTP/2 to call
GotConn as is done directly below.

Fixes #34285

Change-Id: If54bfaf6edb14f5391463f908efbef5bb8a5d78e
GitHub-Last-Rev: 2b7d66a1ce66b4424c4d0fca2b8e8b547d874136
GitHub-Pull-Request: golang/go#34283
Reviewed-on: https://go-review.googlesource.com/c/go/+/195237
Reviewed-by: Michael Fraenkel <michael.fraenkel@gmail.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
(cherry picked from commit 582d5194faec20c475ab93b45cf0520253dec4a9)
Reviewed-on: https://go-review.googlesource.com/c/go/+/196579
Reviewed-by: Daniel Martí <mvdan@mvdan.cc>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>

diff --git a/src/net/http/export_test.go b/src/net/http/export_test.go
index d265cd3f726f1675b2816016a64e295ff9951e91..e5c06a890325647e777e8b4cbf9485b449691bb2 100644 (file)
--- a/src/net/http/export_test.go
+++ b/src/net/http/export_test.go
@@ -208,6 +208,30 @@ func (t *Transport) PutIdleTestConn(scheme, addr string) bool {
        }) == nil
 }
 
+// PutIdleTestConnH2 reports whether it was able to insert a fresh
+// HTTP/2 persistConn for scheme, addr into the idle connection pool.
+func (t *Transport) PutIdleTestConnH2(scheme, addr string, alt RoundTripper) bool {
+       key := connectMethodKey{"", scheme, addr, false}
+
+       if t.MaxConnsPerHost > 0 {
+               // Transport is tracking conns-per-host.
+               // Increment connection count to account
+               // for new persistConn created below.
+               t.connsPerHostMu.Lock()
+               if t.connsPerHost == nil {
+                       t.connsPerHost = make(map[connectMethodKey]int)
+               }
+               t.connsPerHost[key]++
+               t.connsPerHostMu.Unlock()
+       }
+
+       return t.tryPutIdleConn(&persistConn{
+               t:        t,
+               alt:      alt,
+               cacheKey: key,
+       }) == nil
+}
+
 // All test hooks must be non-nil so they can be called directly,
 // but the tests use nil to mean hook disabled.
 func unnilTestHook(f *func()) {

diff --git a/src/net/http/transport.go b/src/net/http/transport.go
index ee279877e02e2cede7ed3e2016ce518642f89966..9c62f0f7797c8d5acde233584c9c2c335907620a 100644 (file)
--- a/src/net/http/transport.go
+++ b/src/net/http/transport.go
@@ -1206,7 +1206,9 @@ func (t *Transport) getConn(treq *transportRequest, cm connectMethod) (pc *persi
        // Queue for idle connection.
        if delivered := t.queueForIdleConn(w); delivered {
                pc := w.pc
-               if trace != nil && trace.GotConn != nil {
+               // Trace only for HTTP/1.
+               // HTTP/2 calls trace.GotConn itself.
+               if pc.alt == nil && trace != nil && trace.GotConn != nil {
                        trace.GotConn(pc.gotIdleConnTrace(pc.idleAt))
                }
                // set request canceler to some non-nil function so we

diff --git a/src/net/http/transport_test.go b/src/net/http/transport_test.go
index 23afff5d843160008b89670a4e5d5a632cfbe40e..746c4c530e30873c5da331084dbd5c9436ab3321 100644 (file)
--- a/src/net/http/transport_test.go
+++ b/src/net/http/transport_test.go
@@ -3562,6 +3562,38 @@ func TestTransportCloseIdleConnsThenReturn(t *testing.T) {
        wantIdle("after final put", 1)
 }
 
+// Test for issue 34282
+// Ensure that getConn doesn't call the GotConn trace hook on a HTTP/2 idle conn
+func TestTransportTraceGotConnH2IdleConns(t *testing.T) {
+       tr := &Transport{}
+       wantIdle := func(when string, n int) bool {
+               got := tr.IdleConnCountForTesting("https", "example.com:443") // key used by PutIdleTestConnH2
+               if got == n {
+                       return true
+               }
+               t.Errorf("%s: idle conns = %d; want %d", when, got, n)
+               return false
+       }
+       wantIdle("start", 0)
+       alt := funcRoundTripper(func() {})
+       if !tr.PutIdleTestConnH2("https", "example.com:443", alt) {
+               t.Fatal("put failed")
+       }
+       wantIdle("after put", 1)
+       ctx := httptrace.WithClientTrace(context.Background(), &httptrace.ClientTrace{
+               GotConn: func(httptrace.GotConnInfo) {
+                       // tr.getConn should leave it for the HTTP/2 alt to call GotConn.
+                       t.Error("GotConn called")
+               },
+       })
+       req, _ := NewRequestWithContext(ctx, MethodGet, "https://example.com", nil)
+       _, err := tr.RoundTrip(req)
+       if err != errFakeRoundTrip {
+               t.Errorf("got error: %v; want %q", err, errFakeRoundTrip)
+       }
+       wantIdle("after round trip", 1)
+}
+
 // This tests that an client requesting a content range won't also
 // implicitly ask for gzip support. If they want that, they need to do it
 // on their own.