[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>

3 files changed, 59 insertions, 1 deletions

diff --git a/src/net/http/export_test.go b/src/net/http/export_test.go
index d265cd3f72..e5c06a8903 100644
--- 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 ee279877e0..9c62f0f779 100644
--- 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 23afff5d84..746c4c530e 100644
--- 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.