http2: close client connections after receiving GOAWAY

Once a connection has received a GOAWAY from the server,
close it after the last outstanding request on the connection
completes.

We're lax about when we call ClientConn.closeConn, frequently
closing the underlying net.Conn multiple times. Stop propagating
errors on closing the net.Conn up through ClientConn.Close and
ClientConn.Shutdown, since these errors are likely to be caused
by double-closing the connection rather than a real fault.

Fixes golang/go#39752.

Change-Id: I06d59e6daa6331c3091e1d49cdbeac313f17e6bd
Reviewed-on: https://go-review.googlesource.com/c/net/+/429060
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Run-TryBot: Damien Neil <dneil@google.com>

Author: neild

http2/transport.go

@@ -258,7 +258,8 @@ func (t *Transport) initConnPool() {
 // HTTP/2 server.
 type ClientConn struct {
 	t             *Transport
-	tconn         net.Conn             // usually *tls.Conn, except specialized impls
+	tconn         net.Conn // usually *tls.Conn, except specialized impls
+	tconnClosed   bool
 	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
@@ -921,10 +922,10 @@ func (cc *ClientConn) onIdleTimeout() {
 	cc.closeIfIdle()
 }
 
-func (cc *ClientConn) closeConn() error {
+func (cc *ClientConn) closeConn() {
 	t := time.AfterFunc(250*time.Millisecond, cc.forceCloseConn)
 	defer t.Stop()
-	return cc.tconn.Close()
+	cc.tconn.Close()
 }
 
 // A tls.Conn.Close can hang for a long time if the peer is unresponsive.
@@ -990,7 +991,8 @@ func (cc *ClientConn) Shutdown(ctx context.Context) error {
 	shutdownEnterWaitStateHook()
 	select {
 	case <-done:
-		return cc.closeConn()
+		cc.closeConn()
+		return nil
 	case <-ctx.Done():
 		cc.mu.Lock()
 		// Free the goroutine above
@@ -1027,32 +1029,33 @@ func (cc *ClientConn) sendGoAway() error {
 
 // closes the client connection immediately. In-flight requests are interrupted.
 // err is sent to streams.
-func (cc *ClientConn) closeForError(err error) error {
+func (cc *ClientConn) closeForError(err error) {
 	cc.mu.Lock()
 	cc.closed = true
 	for _, cs := range cc.streams {
 		cs.abortStreamLocked(err)
 	}
 	cc.cond.Broadcast()
 	cc.mu.Unlock()
-	return cc.closeConn()
+	cc.closeConn()
 }
 
 // Close closes the client connection immediately.
 //
 // In-flight requests are interrupted. For a graceful shutdown, use Shutdown instead.
 func (cc *ClientConn) Close() error {
 	err := errors.New("http2: client connection force closed via ClientConn.Close")
-	return cc.closeForError(err)
+	cc.closeForError(err)
+	return nil
 }
 
 // closes the client connection immediately. In-flight requests are interrupted.
-func (cc *ClientConn) closeForLostPing() error {
+func (cc *ClientConn) closeForLostPing() {
 	err := errors.New("http2: client connection lost")
 	if f := cc.t.CountError; f != nil {
 		f("conn_close_lost_ping")
 	}
-	return cc.closeForError(err)
+	cc.closeForError(err)
 }
 
 // errRequestCanceled is a copy of net/http's errRequestCanceled because it's not
@@ -2005,7 +2008,7 @@ func (cc *ClientConn) forgetStreamID(id uint32) {
 	// wake up RoundTrip if there is a pending request.
 	cc.cond.Broadcast()
 
-	closeOnIdle := cc.singleUse || cc.doNotReuse || cc.t.disableKeepAlives()
+	closeOnIdle := cc.singleUse || cc.doNotReuse || cc.t.disableKeepAlives() || cc.goAway != nil
 	if closeOnIdle && cc.streamsReserved == 0 && len(cc.streams) == 0 {
 		if VerboseLogs {
 			cc.vlogf("http2: Transport closing idle conn %p (forSingleUse=%v, maxStream=%v)", cc, cc.singleUse, cc.nextStreamID-2)
@@ -2674,7 +2677,6 @@ func (rl *clientConnReadLoop) processGoAway(f *GoAwayFrame) error {
 		if fn := cc.t.CountError; fn != nil {
 			fn("recv_goaway_" + f.ErrCode.stringToken())
 		}
-
 	}
 	cc.setGoAway(f)
 	return nil

http2/transport_test.go

@@ -5922,3 +5922,67 @@ func TestTransportSlowWrites(t *testing.T) {
 	}
 	resp.Body.Close()
 }
+
+func TestTransportClosesConnAfterGoAwayNoStreams(t *testing.T) {
+	testTransportClosesConnAfterGoAway(t, 0)
+}
+func TestTransportClosesConnAfterGoAwayLastStream(t *testing.T) {
+	testTransportClosesConnAfterGoAway(t, 1)
+}
+
+// testTransportClosesConnAfterGoAway verifies that the transport
+// closes a connection after reading a GOAWAY from it.
+//
+// lastStream is the last stream ID in the GOAWAY frame.
+// When 0, the transport (unsuccessfully) retries the request (stream 1);
+// when 1, the transport reads the response after receiving the GOAWAY.
+func testTransportClosesConnAfterGoAway(t *testing.T, lastStream uint32) {
+	ct := newClientTester(t)
+
+	var wg sync.WaitGroup
+	wg.Add(1)
+	ct.client = func() error {
+		defer wg.Done()
+		req, _ := http.NewRequest("GET", "https://dummy.tld/", nil)
+		res, err := ct.tr.RoundTrip(req)
+		if err == nil {
+			res.Body.Close()
+		}
+		if gotErr, wantErr := err != nil, lastStream == 0; gotErr != wantErr {
+			t.Errorf("RoundTrip got error %v (want error: %v)", err, wantErr)
+		}
+		if err = ct.cc.Close(); err == nil {
+			err = fmt.Errorf("expected error on Close")
+		} else if strings.Contains(err.Error(), "use of closed network") {
+			err = nil
+		}
+		return err
+	}
+
+	ct.server = func() error {
+		defer wg.Wait()
+		ct.greet()
+		hf, err := ct.firstHeaders()
+		if err != nil {
+			return fmt.Errorf("server failed reading HEADERS: %v", err)
+		}
+		if err := ct.fr.WriteGoAway(lastStream, ErrCodeNo, nil); err != nil {
+			return fmt.Errorf("server failed writing GOAWAY: %v", err)
+		}
+		if lastStream > 0 {
+			// Send a valid response to first request.
+			var buf bytes.Buffer
+			enc := hpack.NewEncoder(&buf)
+			enc.WriteField(hpack.HeaderField{Name: ":status", Value: "200"})
+			ct.fr.WriteHeaders(HeadersFrameParam{
+				StreamID:      hf.StreamID,
+				EndHeaders:    true,
+				EndStream:     true,
+				BlockFragment: buf.Bytes(),
+			})
+		}
+		return nil
+	}
+
+	ct.run()
+}