From ea0da4047cde2179829269cb2eaba24ef1f8a395 Mon Sep 17 00:00:00 2001 From: Damien Neil Date: Fri, 24 Apr 2026 09:01:39 -0700 Subject: [PATCH] net/http/internal/http2: close client conn on GOAWAY with no reqs in-flight When a client connection receives a GOAWAY and there are no in-flight requests, we should immediately close the connection. Previously, we'd only close it if there was at least one in-flight request: the request would be canceled, and the connection would be closed while cleaning up the request. Fixes flaky net/http test TestServerKeepAlivesEnabled/h2, which fails when the GOAWAY arrives after the connection's request has completed, causing the client to skip closing the connection. Includes a small refactor: Move ClientConn.setGoAway into its only caller. Fixes #68440 Change-Id: Ib234a7f15f19803dbd89f323250d90cf6a6a6964 Reviewed-on: https://go-review.googlesource.com/c/go/+/770540 LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com Reviewed-by: Nicholas Husin Reviewed-by: Nicholas Husin Auto-Submit: Damien Neil --- src/net/http/internal/http2/transport.go | 78 ++++++++++--------- src/net/http/internal/http2/transport_test.go | 11 +++ 2 files changed, 52 insertions(+), 37 deletions(-) diff --git a/src/net/http/internal/http2/transport.go b/src/net/http/internal/http2/transport.go index 4639da9789..ae3cf3571a 100644 --- a/src/net/http/internal/http2/transport.go +++ b/src/net/http/internal/http2/transport.go @@ -727,41 +727,6 @@ func (cc *ClientConn) SetDoNotReuse() { cc.doNotReuse = true } -func (cc *ClientConn) setGoAway(f *GoAwayFrame) { - cc.mu.Lock() - defer cc.mu.Unlock() - - old := cc.goAway - cc.goAway = f - - // Merge the previous and current GoAway error frames. - if cc.goAwayDebug == "" { - cc.goAwayDebug = string(f.DebugData()) - } - if old != nil && old.ErrCode != ErrCodeNo { - cc.goAway.ErrCode = old.ErrCode - } - last := f.LastStreamID - for streamID, cs := range cc.streams { - 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) - } - } -} - // CanTakeNewRequest reports whether the connection can take a new request, // meaning it has not been closed or received or sent a GOAWAY. // @@ -2067,6 +2032,9 @@ func (cc *ClientConn) countReadFrameError(err error) { f("read_frame_other") } +// errStopReadLoop is an error which cleanly exits the client's read loop. +var errStopReadLoop = errors.New("client connection is closing (BUG: this is not user visible)") + func (rl *clientConnReadLoop) run() error { cc := rl.cc gotSettings := false @@ -2127,7 +2095,7 @@ func (rl *clientConnReadLoop) run() error { cc.logf("Transport: unhandled response frame type %T", f) } if err != nil { - if VerboseLogs { + if VerboseLogs && err != errStopReadLoop { cc.vlogf("http2: Transport conn %p received error from processing frame %v: %v", cc, summarizeFrame(f), err) } return err @@ -2647,7 +2615,43 @@ func (rl *clientConnReadLoop) processGoAway(f *GoAwayFrame) error { fn("recv_goaway_" + f.ErrCode.stringToken()) } } - cc.setGoAway(f) + + cc.mu.Lock() + defer cc.mu.Unlock() + + old := cc.goAway + cc.goAway = f + + // Merge the previous and current GoAway error frames. + if cc.goAwayDebug == "" { + cc.goAwayDebug = string(f.DebugData()) + } + if old != nil && old.ErrCode != ErrCodeNo { + cc.goAway.ErrCode = old.ErrCode + } + last := f.LastStreamID + if len(cc.streams) == 0 { + // Received a GOAWAY and no streams active, just close the conn. + return errStopReadLoop + } + for streamID, cs := range cc.streams { + 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) + } + } return nil } diff --git a/src/net/http/internal/http2/transport_test.go b/src/net/http/internal/http2/transport_test.go index 955c2208ad..4863054a01 100644 --- a/src/net/http/internal/http2/transport_test.go +++ b/src/net/http/internal/http2/transport_test.go @@ -2215,6 +2215,17 @@ func testTransportUsesGoAwayDebugError(t testing.TB, failMidBody bool) { } } +// https://go.dev/issue/68440 -- receiving a GoAway when there are no outstanding requests +// should immediately close the connection. +func TestTransportGoAwayWithNoConns(t *testing.T) { synctestTest(t, testTransportGoAwayWithNoConns) } +func testTransportGoAwayWithNoConns(t testing.TB) { + tt := newTestTransportWithUnusedConn(t) + tc := tt.getConn() + tc.greet() + tc.writeGoAway(1, ErrCodeNo, nil) + tc.wantClosed() +} + func testTransportReturnsUnusedFlowControl(t testing.TB, oneDataFrame bool) { tc := newTestClientConn(t) tc.greet()