http2: make Transport close unneeded connections after h1->h2 upgrade

If a user starts two HTTP requests when no http2 connection is
available, both end up creating new TCP connections, since the
server's protocol (h1 or h2) isn't yet known. Once it turns out that
the server supports h2, one of the connections is useless. Previously
we kept upgrading both TLS connections to h2 (SETTINGS frame exchange,
etc).  Now the unnecessary connections are closed instead, before the
h2 preface/SETTINGS.

Tests in the standard library (where it's easier to test), in the
commit which updates h2_bundle.go with this change.

Updates golang/go#13957

Change-Id: I5af177e6ea755d572b551cc0b0de9da865ef4ae7
Reviewed-on: https://go-review.googlesource.com/18675Reviewed-by: Andrew Gerrand <adg@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>

http2/client_conn_pool.go

@@ -7,6 +7,7 @@
 package http2
 
 import (
+	"crypto/tls"
 	"net/http"
 	"sync"
 )
@@ -17,21 +18,29 @@ type ClientConnPool interface {
 	MarkDead(*ClientConn)
 }
 
+// TODO: use singleflight for dialing and addConnCalls?
 type clientConnPool struct {
-	t  *Transport
+	t *Transport
+
 	mu sync.Mutex // TODO: maybe switch to RWMutex
 	// TODO: add support for sharing conns based on cert names
 	// (e.g. share conn for googleapis.com and appspot.com)
-	conns   map[string][]*ClientConn // key is host:port
-	dialing map[string]*dialCall     // currently in-flight dials
-	keys    map[*ClientConn][]string
+	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
 }
 
 func (p *clientConnPool) GetClientConn(req *http.Request, addr string) (*ClientConn, error) {
-	return p.getClientConn(req, addr, true)
+	return p.getClientConn(req, addr, dialOnMiss)
 }
 
-func (p *clientConnPool) getClientConn(req *http.Request, addr string, dialOnMiss bool) (*ClientConn, error) {
+const (
+	dialOnMiss   = true
+	noDialOnMiss = false
+)
+
+func (p *clientConnPool) getClientConn(_ *http.Request, addr string, dialOnMiss bool) (*ClientConn, error) {
 	p.mu.Lock()
 	for _, cc := range p.conns[addr] {
 		if cc.CanTakeNewRequest() {
@@ -85,6 +94,64 @@ func (c *dialCall) dial(addr string) {
 	c.p.mu.Unlock()
 }
 
+// addConnIfNeeded makes a NewClientConn out of c if a connection for key doesn't
+// already exist. It coalesces concurrent calls with the same key.
+// This is used by the http1 Transport code when it creates a new connection. Because
+// the http1 Transport doesn't de-dup TCP dials to outbound hosts (because it doesn't know
+// the protocol), it can get into a situation where it has multiple TLS connections.
+// This code decides which ones live or die.
+// The return value used is whether c was used.
+// c is never closed.
+func (p *clientConnPool) addConnIfNeeded(key string, t *Transport, c *tls.Conn) (used bool, err error) {
+	p.mu.Lock()
+	for _, cc := range p.conns[key] {
+		if cc.CanTakeNewRequest() {
+			p.mu.Unlock()
+			return false, nil
+		}
+	}
+	call, dup := p.addConnCalls[key]
+	if !dup {
+		if p.addConnCalls == nil {
+			p.addConnCalls = make(map[string]*addConnCall)
+		}
+		call = &addConnCall{
+			p:    p,
+			done: make(chan struct{}),
+		}
+		p.addConnCalls[key] = call
+		go call.run(t, key, c)
+	}
+	p.mu.Unlock()
+
+	<-call.done
+	if call.err != nil {
+		return false, call.err
+	}
+	return !dup, nil
+}
+
+type addConnCall struct {
+	p    *clientConnPool
+	done chan struct{} // closed when done
+	err  error
+}
+
+func (c *addConnCall) run(t *Transport, key string, tc *tls.Conn) {
+	cc, err := t.NewClientConn(tc)
+
+	p := c.p
+	p.mu.Lock()
+	if err != nil {
+		c.err = err
+	} else {
+		p.addConnLocked(key, cc)
+	}
+	delete(p.addConnCalls, key)
+	p.mu.Unlock()
+	close(c.done)
+}
+
 func (p *clientConnPool) addConn(key string, cc *ClientConn) {
 	p.mu.Lock()
 	p.addConnLocked(key, cc)

http2/configure_transport.go

@@ -28,12 +28,17 @@ func configureTransport(t1 *http.Transport) error {
 		t1.TLSClientConfig.NextProtos = append(t1.TLSClientConfig.NextProtos, "http/1.1")
 	}
 	upgradeFn := func(authority string, c *tls.Conn) http.RoundTripper {
-		cc, err := t2.NewClientConn(c)
-		if err != nil {
-			c.Close()
+		addr := authorityAddr(authority)
+		if used, err := connPool.addConnIfNeeded(addr, t2, c); err != nil {
+			go c.Close()
 			return erringRoundTripper{err}
+		} else if !used {
+			// Turns out we don't need this c.
+			// For example, two goroutines made requests to the same host
+			// at the same time, both kicking off TCP dials. (since protocol
+			// was unknown)
+			go c.Close()
 		}
-		connPool.addConn(authorityAddr(authority), cc)
 		return t2
 	}
 	if m := t1.TLSNextProto; len(m) == 0 {
@@ -64,8 +69,7 @@ func registerHTTPSProtocol(t *http.Transport, rt http.RoundTripper) (err error)
 type noDialClientConnPool struct{ *clientConnPool }
 
 func (p noDialClientConnPool) GetClientConn(req *http.Request, addr string) (*ClientConn, error) {
-	const doDial = false
-	return p.getClientConn(req, addr, doDial)
+	return p.getClientConn(req, addr, noDialOnMiss)
 }
 
 // noDialH2RoundTripper is a RoundTripper which only tries to complete the request