[release-branch.go1.13] net/http: don't cache http2.erringRoundTripper connections

Fixes #35087
Updates #34978

Change-Id: I3baf1392ba7366ae6628889c47c343ef702ec438
Reviewed-on: https://go-review.googlesource.com/c/go/+/202078
Reviewed-by: Bryan C. Mills <bcmills@google.com>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
(cherry picked from commit 88186e5e232625f9c91d639e0cb90a88c6cf1172)
Reviewed-on: https://go-review.googlesource.com/c/go/+/202642
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>

diff --git a/src/net/http/transport.go b/src/net/http/transport.go
index caa4bdc36ff01c2b9d84b62759b7cd1c0ba31455..b46c4e7066c4865e86a2b3c55b2aa853009b28f7 100644 (file)
--- a/src/net/http/transport.go
+++ b/src/net/http/transport.go
@@ -537,10 +537,15 @@ func (t *Transport) roundTrip(req *Request) (*Response, error) {
                if err == nil {
                        return resp, nil
                }
-               if http2isNoCachedConnError(err) {
+
+               // Failed. Clean up and determine whether to retry.
+
+               _, isH2DialError := pconn.alt.(http2erringRoundTripper)
+               if http2isNoCachedConnError(err) || isH2DialError {
                        t.removeIdleConn(pconn)
                        t.decConnsPerHost(pconn.cacheKey)
-               } else if !pconn.shouldRetryRequest(req, err) {
+               }
+               if !pconn.shouldRetryRequest(req, err) {
                        // Issue 16465: return underlying net.Conn.Read error from peek,
                        // as we've historically done.
                        if e, ok := err.(transportReadFromServerError); ok {

diff --git a/src/net/http/transport_test.go b/src/net/http/transport_test.go
index e43c8956ee0a3b704da03ee6dee8d0837e6e530c..f8c13a70917a6458917e40b8a0ebb35f980f15da 100644 (file)
--- a/src/net/http/transport_test.go
+++ b/src/net/http/transport_test.go
@@ -5719,3 +5719,78 @@ func TestInvalidHeaderResponse(t *testing.T) {
                t.Errorf(`bad "Foo " header value: %q, want %q`, v, "bar")
        }
 }
+
+// breakableConn is a net.Conn wrapper with a Write method
+// that will fail when its brokenState is true.
+type breakableConn struct {
+       net.Conn
+       *brokenState
+}
+
+type brokenState struct {
+       sync.Mutex
+       broken bool
+}
+
+func (w *breakableConn) Write(b []byte) (n int, err error) {
+       w.Lock()
+       defer w.Unlock()
+       if w.broken {
+               return 0, errors.New("some write error")
+       }
+       return w.Conn.Write(b)
+}
+
+// Issue 34978: don't cache a broken HTTP/2 connection
+func TestDontCacheBrokenHTTP2Conn(t *testing.T) {
+       cst := newClientServerTest(t, h2Mode, HandlerFunc(func(w ResponseWriter, r *Request) {}), optQuietLog)
+       defer cst.close()
+
+       var brokenState brokenState
+
+       cst.tr.Dial = func(netw, addr string) (net.Conn, error) {
+               c, err := net.Dial(netw, addr)
+               if err != nil {
+                       t.Errorf("unexpected Dial error: %v", err)
+                       return nil, err
+               }
+               return &breakableConn{c, &brokenState}, err
+       }
+
+       const numReqs = 5
+       var gotConns uint32 // atomic
+       for i := 1; i <= numReqs; i++ {
+               brokenState.Lock()
+               brokenState.broken = false
+               brokenState.Unlock()
+
+               // doBreak controls whether we break the TCP connection after the TLS
+               // handshake (before the HTTP/2 handshake). We test a few failures
+               // in a row followed by a final success.
+               doBreak := i != numReqs
+
+               ctx := httptrace.WithClientTrace(context.Background(), &httptrace.ClientTrace{
+                       GotConn: func(info httptrace.GotConnInfo) {
+                               atomic.AddUint32(&gotConns, 1)
+                       },
+                       TLSHandshakeDone: func(cfg tls.ConnectionState, err error) {
+                               brokenState.Lock()
+                               defer brokenState.Unlock()
+                               if doBreak {
+                                       brokenState.broken = true
+                               }
+                       },
+               })
+               req, err := NewRequestWithContext(ctx, "GET", cst.ts.URL, nil)
+               if err != nil {
+                       t.Fatal(err)
+               }
+               _, err = cst.c.Do(req)
+               if doBreak != (err != nil) {
+                       t.Errorf("for iteration %d, doBreak=%v; unexpected error %v", i, doBreak, err)
+               }
+       }
+       if got, want := atomic.LoadUint32(&gotConns), 1; int(got) != want {
+               t.Errorf("GotConn calls = %v; want %v", got, want)
+       }
+}