http2: make Transport honor Request.Close more aggressively

The old support for Request.Close was optimistic, but there were still
windows where two connections should share the same
connection. Instead, support it earlier and more explicitly.

This fixes a flaky test on FreeBSD which was probably only flaky due
to unfortunate scheduling/timing.

Change-Id: I2eed9e59116356257e1b7df7a034b1c5d5f6533a
Reviewed-on: https://go-review.googlesource.com/24810
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>

http2/client_conn_pool.go

@@ -52,7 +52,16 @@ const (
 	noDialOnMiss = false
 )
 
-func (p *clientConnPool) getClientConn(_ *http.Request, addr string, dialOnMiss bool) (*ClientConn, error) {
+func (p *clientConnPool) getClientConn(req *http.Request, addr string, dialOnMiss bool) (*ClientConn, error) {
+	if req.Close && dialOnMiss {
+		// It gets its own connection.
+		cc, err := p.t.dialClientConn(addr)
+		if err != nil {
+			return nil, err
+		}
+		cc.singleUse = true
+		return cc, nil
+	}
 	p.mu.Lock()
 	for _, cc := range p.conns[addr] {
 		if cc.CanTakeNewRequest() {

http2/server_test.go

@@ -104,6 +104,8 @@ func newServerTester(t testing.TB, handler http.HandlerFunc, opts ...interface{}
 			case optQuiet:
 				quiet = true
 			}
+		case func(net.Conn, http.ConnState):
+			ts.Config.ConnState = v
 		default:
 			t.Fatalf("unknown newServerTester option type %T", v)
 		}

http2/transport.go

@@ -139,9 +139,10 @@ 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
+	t         *Transport
+	tconn     net.Conn             // usually *tls.Conn, except specialized impls
+	tlsState  *tls.ConnectionState // nil only for specialized impls
+	singleUse bool                 // whether being used for a single http.Request
 
 	// readLoop goroutine fields:
 	readerDone chan struct{} // closed on error
@@ -515,6 +516,9 @@ func (cc *ClientConn) CanTakeNewRequest() bool {
 }
 
 func (cc *ClientConn) canTakeNewRequestLocked() bool {
+	if cc.singleUse && cc.nextStreamID > 1 {
+		return false
+	}
 	return cc.goAway == nil && !cc.closed &&
 		int64(len(cc.streams)+1) < int64(cc.maxConcurrentStreams) &&
 		cc.nextStreamID < 2147483647
@@ -1223,7 +1227,7 @@ func (rl *clientConnReadLoop) cleanup() {
 
 func (rl *clientConnReadLoop) run() error {
 	cc := rl.cc
-	rl.closeWhenIdle = cc.t.disableKeepAlives()
+	rl.closeWhenIdle = cc.t.disableKeepAlives() || cc.singleUse
 	gotReply := false // ever saw a reply
 	for {
 		f, err := cc.fr.ReadFrame()

http2/transport_test.go

@@ -150,7 +150,9 @@ func TestTransport(t *testing.T) {
 func onSameConn(t *testing.T, modReq func(*http.Request)) bool {
 	st := newServerTester(t, func(w http.ResponseWriter, r *http.Request) {
 		io.WriteString(w, r.RemoteAddr)
-	}, optOnlyServer)
+	}, optOnlyServer, func(c net.Conn, st http.ConnState) {
+		t.Logf("conn %v is now state %v", c.RemoteAddr(), st)
+	})
 	defer st.Close()
 	tr := &Transport{TLSClientConfig: tlsConfigInsecure}
 	defer tr.CloseIdleConnections()