http2: don't retry the first request on a connection on GOAWAY error

When a server sends a GOAWAY frame, it indicates the ID of the
last stream it processed. We use this to mark any requests after
that stream as being safe to retry on a new connection.

Change this to not retry the first request on a connection if we
get a GOAWAY with an error, even if the GOAWAY has a stream ID
of 0 indicating that it didn't process that request.
If we're getting an error as the first result on a new connection,
then there's either something wrong with the server or something
wrong with our request; either way, retrying isn't likely to be
productive and may be unsafe.

This matches the behavior of the HTTP/1 client, which
also avoids retrying the first request on a new connection.

For golang/go#66668
Fixes golang/go#60636

Change-Id: I90ea7cfce2974dd413f7cd8b78541678850376a5
Reviewed-on: https://go-review.googlesource.com/c/net/+/576895
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Jonathan Amsterdam <[email protected]>

Author: neild

http2/transport.go

@@ -936,7 +936,20 @@ func (cc *ClientConn) setGoAway(f *GoAwayFrame) {
 	}
 	last := f.LastStreamID
 	for streamID, cs := range cc.streams {
-		if streamID > last {
+		if streamID <= last {
+			// The server's GOAWAY indicates that it received this stream.
+			// It will either finish processing it, or close the connection
+			// without doing so. Either way, leave the stream alone for now.
+			continue
+		}
+		if streamID == 1 && cc.goAway.ErrCode != ErrCodeNo {
+			// Don't retry the first stream on a connection if we get a non-NO error.
+			// If the server is sending an error on a new connection,
+			// retrying the request on a new one probably isn't going to work.
+			cs.abortStreamLocked(fmt.Errorf("http2: Transport received GOAWAY from server ErrCode:%v", cc.goAway.ErrCode))
+		} else {
+			// Aborting the stream with errClentConnGotGoAway indicates that
+			// the request should be retried on a new connection.
 			cs.abortStreamLocked(errClientConnGotGoAway)
 		}
 	}

http2/transport_test.go

@@ -3144,13 +3144,40 @@ func TestTransportPingWhenReadingPingDisabled(t *testing.T) {
 	}
 }
 
-func TestTransportRetryAfterGOAWAY(t *testing.T) {
+func TestTransportRetryAfterGOAWAYNoRetry(t *testing.T) {
 	tt := newTestTransport(t)
 
 	req, _ := http.NewRequest("GET", "https://dummy.tld/", nil)
 	rt := tt.roundTrip(req)
 
-	// First attempt: Server sends a GOAWAY.
+	// First attempt: Server sends a GOAWAY with an error and
+	// a MaxStreamID less than the request ID.
+	// This probably indicates that there was something wrong with our request,
+	// so we don't retry it.
+	tc := tt.getConn()
+	tc.wantFrameType(FrameSettings)
+	tc.wantFrameType(FrameWindowUpdate)
+	tc.wantHeaders(wantHeader{
+		streamID:  1,
+		endStream: true,
+	})
+	tc.writeSettings()
+	tc.writeGoAway(0 /*max id*/, ErrCodeInternal, nil)
+	if rt.err() == nil {
+		t.Fatalf("after GOAWAY, RoundTrip is not done, want error")
+	}
+}
+
+func TestTransportRetryAfterGOAWAYRetry(t *testing.T) {
+	tt := newTestTransport(t)
+
+	req, _ := http.NewRequest("GET", "https://dummy.tld/", nil)
+	rt := tt.roundTrip(req)
+
+	// First attempt: Server sends a GOAWAY with ErrCodeNo and
+	// a MaxStreamID less than the request ID.
+	// We take the server at its word that nothing has really gone wrong,
+	// and retry the request.
 	tc := tt.getConn()
 	tc.wantFrameType(FrameSettings)
 	tc.wantFrameType(FrameWindowUpdate)
@@ -3185,6 +3212,69 @@ func TestTransportRetryAfterGOAWAY(t *testing.T) {
 	rt.wantStatus(200)
 }
 
+func TestTransportRetryAfterGOAWAYSecondRequest(t *testing.T) {
+	tt := newTestTransport(t)
+
+	// First request succeeds.
+	req, _ := http.NewRequest("GET", "https://dummy.tld/", nil)
+	rt1 := tt.roundTrip(req)
+	tc := tt.getConn()
+	tc.wantFrameType(FrameSettings)
+	tc.wantFrameType(FrameWindowUpdate)
+	tc.wantHeaders(wantHeader{
+		streamID:  1,
+		endStream: true,
+	})
+	tc.writeSettings()
+	tc.wantFrameType(FrameSettings) // Settings ACK
+	tc.writeHeaders(HeadersFrameParam{
+		StreamID:   1,
+		EndHeaders: true,
+		EndStream:  true,
+		BlockFragment: tc.makeHeaderBlockFragment(
+			":status", "200",
+		),
+	})
+	rt1.wantStatus(200)
+
+	// Second request: Server sends a GOAWAY with
+	// a MaxStreamID less than the request ID.
+	// The server says it didn't see this request,
+	// so we retry it on a new connection.
+	req, _ = http.NewRequest("GET", "https://dummy.tld/", nil)
+	rt2 := tt.roundTrip(req)
+
+	// Second request, first attempt.
+	tc.wantHeaders(wantHeader{
+		streamID:  3,
+		endStream: true,
+	})
+	tc.writeSettings()
+	tc.writeGoAway(1 /*max id*/, ErrCodeProtocol, nil)
+	if rt2.done() {
+		t.Fatalf("after GOAWAY, RoundTrip is done; want it to be retrying")
+	}
+
+	// Second request, second attempt.
+	tc = tt.getConn()
+	tc.wantFrameType(FrameSettings)
+	tc.wantFrameType(FrameWindowUpdate)
+	tc.wantHeaders(wantHeader{
+		streamID:  1,
+		endStream: true,
+	})
+	tc.writeSettings()
+	tc.writeHeaders(HeadersFrameParam{
+		StreamID:   1,
+		EndHeaders: true,
+		EndStream:  true,
+		BlockFragment: tc.makeHeaderBlockFragment(
+			":status", "200",
+		),
+	})
+	rt2.wantStatus(200)
+}
+
 func TestTransportRetryAfterRefusedStream(t *testing.T) {
 	tt := newTestTransport(t)