From ec05fdcd71141c885f3fb84c41d1c692f094ccbe Mon Sep 17 00:00:00 2001 From: Damien Neil Date: Fri, 5 Apr 2024 08:07:32 -0700 Subject: [PATCH] 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 Reviewed-by: Jonathan Amsterdam --- http2/transport.go | 15 ++++++- http2/transport_test.go | 94 ++++++++++++++++++++++++++++++++++++++++- 2 files changed, 106 insertions(+), 3 deletions(-) diff --git a/http2/transport.go b/http2/transport.go index ce375c8c7..2fa49490c 100644 --- a/http2/transport.go +++ b/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) } } diff --git a/http2/transport_test.go b/http2/transport_test.go index 11ff67b4c..3e4297f28 100644 --- a/http2/transport_test.go +++ b/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)