http2: avoid extra GetConn trace call

CL 352469 inverts the case in shouldTraceGetConn: We want to call GetConn
for connections that have been previously used, but it calls GetConn
only on approximately the first use. "Approximately", because it uses
cc.nextStreamID to determine if the connection has been used, which
is racy.

Restructure the decision to call GetConn to track a per-ClientConn bool
indicating whether GetConn has already been called for this connection.
Set this bool for connections received from net/http, clear it after the
first use of the connection.

Fixes net/http's TestTransportEventTrace_h2.

Change-Id: I8e3dbba7cfbce9acd3612e39b6b6ee558bbfc864
Reviewed-on: https://go-review.googlesource.com/c/net/+/353875
Trust: Damien Neil <dneil@google.com>
Run-TryBot: Damien Neil <dneil@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>

http2/client_conn_pool.go

@@ -48,7 +48,7 @@ type clientConnPool struct {
 	conns        map[string][]*ClientConn // key is host:port
 	dialing      map[string]*dialCall     // currently in-flight dials
 	keys         map[*ClientConn][]string
-	addConnCalls map[string]*addConnCall // in-flight addConnIfNeede calls
+	addConnCalls map[string]*addConnCall // in-flight addConnIfNeeded calls
 }
 
 func (p *clientConnPool) GetClientConn(req *http.Request, addr string) (*ClientConn, error) {
@@ -60,29 +60,6 @@ const (
 	noDialOnMiss = false
 )
 
-// shouldTraceGetConn reports whether getClientConn should call any
-// ClientTrace.GetConn hook associated with the http.Request.
-//
-// This complexity is needed to avoid double calls of the GetConn hook
-// during the back-and-forth between net/http and x/net/http2 (when the
-// net/http.Transport is upgraded to also speak http2), as well as support
-// the case where x/net/http2 is being used directly.
-func (p *clientConnPool) shouldTraceGetConn(cc *ClientConn) bool {
-	// If our Transport wasn't made via ConfigureTransport, always
-	// trace the GetConn hook if provided, because that means the
-	// http2 package is being used directly and it's the one
-	// dialing, as opposed to net/http.
-	if _, ok := p.t.ConnPool.(noDialClientConnPool); !ok {
-		return true
-	}
-	// Otherwise, only use the GetConn hook if this connection has
-	// been used previously for other requests. For fresh
-	// connections, the net/http package does the dialing.
-	cc.mu.Lock()
-	defer cc.mu.Unlock()
-	return cc.nextStreamID == 1
-}
-
 func (p *clientConnPool) getClientConn(req *http.Request, addr string, dialOnMiss bool) (*ClientConn, error) {
 	// TODO(dneil): Dial a new connection when t.DisableKeepAlives is set?
 	if isConnectionCloseRequest(req) && dialOnMiss {
@@ -99,9 +76,13 @@ func (p *clientConnPool) getClientConn(req *http.Request, addr string, dialOnMis
 		p.mu.Lock()
 		for _, cc := range p.conns[addr] {
 			if cc.ReserveNewRequest() {
-				if p.shouldTraceGetConn(cc) {
+				// When a connection is presented to us by the net/http package,
+				// the GetConn hook has already been called.
+				// Don't call it a second time here.
+				if !cc.getConnCalled {
 					traceGetConn(req, addr)
 				}
+				cc.getConnCalled = false
 				p.mu.Unlock()
 				return cc, nil
 			}
@@ -214,6 +195,7 @@ type addConnCall struct {
 
 func (c *addConnCall) run(t *Transport, key string, tc *tls.Conn) {
 	cc, err := t.NewClientConn(tc)
+	cc.getConnCalled = true // already called by the net/http package
 
 	p := c.p
 	p.mu.Lock()

http2/transport.go

@@ -242,11 +242,12 @@ func (t *Transport) initConnPool() {
 // ClientConn is the state of a single HTTP/2 client connection to an
 // HTTP/2 server.
 type ClientConn struct {
-	t         *Transport
-	tconn     net.Conn             // usually *tls.Conn, except specialized impls
-	tlsState  *tls.ConnectionState // nil only for specialized impls
-	reused    uint32               // whether conn is being reused; atomic
-	singleUse bool                 // whether being used for a single http.Request
+	t             *Transport
+	tconn         net.Conn             // usually *tls.Conn, except specialized impls
+	tlsState      *tls.ConnectionState // nil only for specialized impls
+	reused        uint32               // whether conn is being reused; atomic
+	singleUse     bool                 // whether being used for a single http.Request
+	getConnCalled bool                 // used by clientConnPool
 
 	// readLoop goroutine fields:
 	readerDone chan struct{} // closed on error
@@ -762,7 +763,6 @@ func (cc *ClientConn) ReserveNewRequest() bool {
 // connection to initiate a new RoundTrip request.
 type clientConnIdleState struct {
 	canTakeNewRequest bool
-	freshConn         bool // whether it's unused by any previous request
 }
 
 func (cc *ClientConn) idleState() clientConnIdleState {
@@ -790,7 +790,6 @@ func (cc *ClientConn) idleStateLocked() (st clientConnIdleState) {
 		!cc.doNotReuse &&
 		int64(cc.nextStreamID)+2*int64(cc.pendingRequests) < math.MaxInt32 &&
 		!cc.tooIdleLocked()
-	st.freshConn = cc.nextStreamID == 1 && st.canTakeNewRequest
 	return
 }
 

http2/transport_test.go

@@ -243,6 +243,68 @@ func TestTransportReusesConn_ConnClose(t *testing.T) {
 	}
 }
 
+func TestTransportGetGotConnHooks_HTTP2Transport(t *testing.T) {
+	testTransportGetGotConnHooks(t, false)
+}
+func TestTransportGetGotConnHooks_Client(t *testing.T) { testTransportGetGotConnHooks(t, true) }
+
+func testTransportGetGotConnHooks(t *testing.T, useClient bool) {
+	st := newServerTester(t, func(w http.ResponseWriter, r *http.Request) {
+		io.WriteString(w, r.RemoteAddr)
+	}, func(s *httptest.Server) {
+		s.EnableHTTP2 = true
+	}, optOnlyServer)
+	defer st.Close()
+
+	tr := &Transport{TLSClientConfig: tlsConfigInsecure}
+	client := st.ts.Client()
+	ConfigureTransports(client.Transport.(*http.Transport))
+
+	var (
+		getConns int32
+		gotConns int32
+	)
+	for i := 0; i < 2; i++ {
+		trace := &httptrace.ClientTrace{
+			GetConn: func(hostport string) {
+				atomic.AddInt32(&getConns, 1)
+			},
+			GotConn: func(connInfo httptrace.GotConnInfo) {
+				got := atomic.AddInt32(&gotConns, 1)
+				wantReused, wantWasIdle := false, false
+				if got > 1 {
+					wantReused, wantWasIdle = true, true
+				}
+				if connInfo.Reused != wantReused || connInfo.WasIdle != wantWasIdle {
+					t.Errorf("GotConn %v: Reused=%v (want %v), WasIdle=%v (want %v)", i, connInfo.Reused, wantReused, connInfo.WasIdle, wantWasIdle)
+				}
+			},
+		}
+		req, err := http.NewRequest("GET", st.ts.URL, nil)
+		if err != nil {
+			t.Fatal(err)
+		}
+		req = req.WithContext(httptrace.WithClientTrace(req.Context(), trace))
+
+		var res *http.Response
+		if useClient {
+			res, err = client.Do(req)
+		} else {
+			res, err = tr.RoundTrip(req)
+		}
+		if err != nil {
+			t.Fatal(err)
+		}
+		res.Body.Close()
+		if get := atomic.LoadInt32(&getConns); get != int32(i+1) {
+			t.Errorf("after request %v, %v calls to GetConns: want %v", i, get, i+1)
+		}
+		if got := atomic.LoadInt32(&gotConns); got != int32(i+1) {
+			t.Errorf("after request %v, %v calls to GotConns: want %v", i, got, i+1)
+		}
+	}
+}
+
 // Tests that the Transport only keeps one pending dial open per destination address.
 // https://golang.org/issue/13397
 func TestTransportGroupsPendingDials(t *testing.T) {