From 7458cdac50723469fc35c2f8d697f5941819ccc2 Mon Sep 17 00:00:00 2001 From: Ali Sayyah Date: Mon, 17 Aug 2026 12:30:32 -0700 Subject: [PATCH] net/http: count a lost HTTP/2 ping exactly once, and never on a failed connection The HTTP/2 health-check timer is re-armed after every ReadFrame return, including the final erroring one, and no close path stops it: one SendPingTimeout after every connection close a post-mortem health check pings the dead connection, fails immediately, and reports a spurious CountError("conn_close_lost_ping"). A genuinely lost ping is counted twice: the real detection, then the post-close echo. Stop the timer when the read loop exits, publish the read loop's terminal exit under cc.mu before the failure is counted, and make lost-ping classification a single-claim close: the eligibility check and the claim share one critical section in closeForLostPing, so overlapping health checks, or a ping racing the read loop's terminal exit, can never produce a second count. The CountError callback runs outside the lock. A ping that fails on a connection that is live at claim time is still counted and still closes the connection, preserving write-blocked-ping detection. The equivalent x/net change (golang/net#262) fixes the legacy transport used on Go versions before 1.27 and with the http2legacy build tag. Fixes #80920 --- .../http/internal/http2/clientconn_test.go | 7 + src/net/http/internal/http2/export_test.go | 26 ++- src/net/http/internal/http2/transport.go | 53 +++++- src/net/http/internal/http2/transport_test.go | 164 ++++++++++++++++++ 4 files changed, 245 insertions(+), 5 deletions(-) diff --git a/src/net/http/internal/http2/clientconn_test.go b/src/net/http/internal/http2/clientconn_test.go index a99ab784a720db..626c4f91257d94 100644 --- a/src/net/http/internal/http2/clientconn_test.go +++ b/src/net/http/internal/http2/clientconn_test.go @@ -562,6 +562,8 @@ func newTestTransport(t *testing.T, opts ...any) *testTransport { tr1.HTTP2 = &http.HTTP2Config{} } o(tr1.HTTP2) + case func(*Transport): + // Applied below, once the HTTP/2 transport exists. default: t.Fatalf("unknown newTestTransport option type %T", o) } @@ -569,6 +571,11 @@ func newTestTransport(t *testing.T, opts ...any) *testTransport { tt.tr1 = tr1 tr2 := transportFromH1Transport(tr1).(*Transport) + for _, o := range opts { + if f, ok := o.(func(*Transport)); ok { + f(tr2) + } + } tr2.TestSetNewClientConnHook(func(cc *ClientConn) { tc := newTestClientConnFromClientConn(t, tr2, cc) tt.ccs = append(tt.ccs, tc) diff --git a/src/net/http/internal/http2/export_test.go b/src/net/http/internal/http2/export_test.go index e9d7c6bead5d9d..400577931d7721 100644 --- a/src/net/http/internal/http2/export_test.go +++ b/src/net/http/internal/http2/export_test.go @@ -116,11 +116,33 @@ func (t *Transport) TestNewClientConn(c net.Conn, singleUse bool, internalStateH } func (t *Transport) TestSetNewClientConnHook(f func(*ClientConn)) { - t.transportTestHooks = &transportTestHooks{ - newclientconn: f, + if t.transportTestHooks == nil { + t.transportTestHooks = &transportTestHooks{} } + t.transportTestHooks.newclientconn = f } +// TestSetReadLoopExitedHook installs f to run on a connection's read-loop +// goroutine after its terminal exit is published and before its cleanup. +func (t *Transport) TestSetReadLoopExitedHook(f func(*ClientConn)) { + if t.transportTestHooks == nil { + t.transportTestHooks = &transportTestHooks{} + } + t.transportTestHooks.readLoopExited = f +} + +// TestSetNewHealthCheckTimerHook installs f to observe each connection's +// health-check timer as it is created. +func (t *Transport) TestSetNewHealthCheckTimerHook(f func(*time.Timer)) { + if t.transportTestHooks == nil { + t.transportTestHooks = &transportTestHooks{} + } + t.transportTestHooks.newHealthCheckTimer = f +} + +// TestCloseForLostPing invokes the lost-ping close claim directly. +func (cc *ClientConn) TestCloseForLostPing() { cc.closeForLostPing() } + func (cc *ClientConn) TestNetConn() net.Conn { return cc.tconn } func (cc *ClientConn) TestSetNetConn(c net.Conn) { cc.tconn = c } diff --git a/src/net/http/internal/http2/transport.go b/src/net/http/internal/http2/transport.go index 9d63ed26c85cfc..e0dbe9e6a64826 100644 --- a/src/net/http/internal/http2/transport.go +++ b/src/net/http/internal/http2/transport.go @@ -77,6 +77,12 @@ type Transport struct { type transportTestHooks struct { newclientconn func(*ClientConn) + // readLoopExited, if non-nil, runs on a connection's read-loop goroutine + // after the loop publishes its terminal exit and before its cleanup. + readLoopExited func(*ClientConn) + // newHealthCheckTimer, if non-nil, observes each connection's + // health-check timer as it is created. + newHealthCheckTimer func(*time.Timer) } func (t *Transport) maxHeaderListSize() uint32 { @@ -141,6 +147,12 @@ type ClientConn struct { readerDone chan struct{} // closed on error readerErr error // set before readerDone is closed + // readLoopExited is owned by mu and set once the read loop has observed + // a terminal connection error, before cleanup publishes closed. Lost-ping + // classification consults it so a concurrently failing ping is never + // counted after the connection has terminally failed. + readLoopExited bool + idleTimeout time.Duration // or 0 for never idleTimer *time.Timer @@ -660,7 +672,7 @@ func (t *Transport) newClientConn(c net.Conn, singleUse bool, internalStateHook lastActive: time.Now(), internalStateHook: internalStateHook, } - if t.transportTestHooks != nil { + if t.transportTestHooks != nil && t.transportTestHooks.newclientconn != nil { t.transportTestHooks.newclientconn(cc) c = cc.tconn } @@ -1072,13 +1084,29 @@ func (cc *ClientConn) Close() error { return nil } -// closes the client connection immediately. In-flight requests are interrupted. +// closeForLostPing closes the client connection if this health check is the +// one that claims it. The eligibility check and the claim share one critical +// section: a connection that is already closed, whose read loop has published +// a terminal failure, or that an overlapping health check already claimed is +// not a lost ping and is not counted again. In-flight requests on a claimed +// connection are interrupted. func (cc *ClientConn) closeForLostPing() { err := errors.New("http2: client connection lost") + cc.mu.Lock() + if cc.closed || cc.readLoopExited { + cc.mu.Unlock() + return + } + cc.closed = true + for _, cs := range cc.streams { + cs.abortStreamLocked(err) + } + cc.cond.Broadcast() + cc.mu.Unlock() if f := cc.fr.countError; f != nil { f("conn_close_lost_ping") } - cc.closeForError(err) + cc.closeConn() } // errRequestCanceled is a copy of net/http's errRequestCanceled because it's not @@ -2034,6 +2062,12 @@ func (cc *ClientConn) readLoop() { rl := &clientConnReadLoop{cc: cc} defer rl.cleanup() cc.readerErr = rl.run() + cc.mu.Lock() + cc.readLoopExited = true + cc.mu.Unlock() + if hooks := cc.t.transportTestHooks; hooks != nil && hooks.readLoopExited != nil { + hooks.readLoopExited(cc) + } if ce, ok := cc.readerErr.(ConnectionError); ok { cc.wmu.Lock() cc.fr.WriteGoAway(0, ErrCode(ce), nil) @@ -2165,6 +2199,13 @@ func (rl *clientConnReadLoop) run() error { var t *time.Timer if readIdleTimeout != 0 { t = time.AfterFunc(readIdleTimeout, cc.healthCheck) + if hooks := cc.t.transportTestHooks; hooks != nil && hooks.newHealthCheckTimer != nil { + hooks.newHealthCheckTimer(t) + } + // The timer is re-armed below after every ReadFrame return, including + // the final erroring one; without a Stop it would outlive the + // connection and health-check the closed conn once more. + defer t.Stop() } for { f, err := cc.fr.ReadFrame() @@ -2183,6 +2224,12 @@ func (rl *clientConnReadLoop) run() error { } continue } else if err != nil { + // Publish the terminal exit before counting, so a concurrently + // failing health-check ping cannot also classify this failure + // as a lost ping. + cc.mu.Lock() + cc.readLoopExited = true + cc.mu.Unlock() cc.countReadFrameError(err) return err } diff --git a/src/net/http/internal/http2/transport_test.go b/src/net/http/internal/http2/transport_test.go index 6cbd8cb676a431..b5c6e7a3f05f37 100644 --- a/src/net/http/internal/http2/transport_test.go +++ b/src/net/http/internal/http2/transport_test.go @@ -2863,6 +2863,170 @@ func testTransportCloseAfterLostPing(t *testing.T) { } } +// A connection close must not surface as a lost ping: the health-check timer +// used to survive the read loop's exit and re-ping the dead connection once, +// counting a spurious conn_close_lost_ping ~SendPingTimeout after every close. +func TestTransportNoLostPingAfterConnClose(t *testing.T) { + synctest.Test(t, testTransportNoLostPingAfterConnClose) +} +func testTransportNoLostPingAfterConnClose(t *testing.T) { + var lostPings atomic.Int64 + var hcTimer atomic.Pointer[time.Timer] + tc := newTestClientConn(t, func(h2 *http.HTTP2Config) { + h2.PingTimeout = 1 * time.Second + h2.SendPingTimeout = 1 * time.Second + h2.CountError = func(errType string) { + if errType == "conn_close_lost_ping" { + lostPings.Add(1) + } + } + }, func(tr *Transport) { + tr.TestSetNewHealthCheckTimerHook(func(tm *time.Timer) { + hcTimer.Store(tm) + }) + }) + tc.greet() + + tc.closeWrite() + // The timer must be disarmed at the exit itself, checked before the + // instant it would otherwise fire. + time.Sleep(1 * time.Millisecond) + tm := hcTimer.Load() + if tm == nil { + t.Fatalf("health-check timer was never created") + } + if tm.Stop() { + t.Errorf("health-check timer still armed after the read loop exited") + } + time.Sleep(3 * time.Second) + if n := lostPings.Load(); n != 0 { + t.Errorf("conn_close_lost_ping count = %d after connection close, want 0", n) + } +} + +// A genuine lost ping on a live connection is detected and counted exactly +// once: neither the teardown it triggers nor a later wake of the read loop +// may re-report the now-closed connection as a second lost ping. +func TestTransportLostPingCountedOnce(t *testing.T) { + synctest.Test(t, testTransportLostPingCountedOnce) +} +func testTransportLostPingCountedOnce(t *testing.T) { + var lostPings atomic.Int64 + tc := newTestClientConn(t, func(h2 *http.HTTP2Config) { + h2.PingTimeout = 1 * time.Second + h2.SendPingTimeout = 1 * time.Second + h2.CountError = func(errType string) { + if errType == "conn_close_lost_ping" { + lostPings.Add(1) + } + } + }) + tc.greet() + + // No frames for SendPingTimeout: a health-check ping goes out and is + // never answered. + time.Sleep(1 * time.Second) + tc.wantFrameType(FramePing) + // Past the ping deadline, not at it: sleeping to the exact instant races + // the health check's own timeout processing under synctest. + time.Sleep(1*time.Second + 1*time.Millisecond) + if n := lostPings.Load(); n != 1 { + t.Fatalf("conn_close_lost_ping count = %d after a lost ping, want 1", n) + } + // Wake the read loop's blocked read so it observes the teardown; the + // exit must not re-arm the health check into a second, phantom count. + tc.closeWrite() + time.Sleep(3 * time.Second) + if n := lostPings.Load(); n != 1 { + t.Errorf("conn_close_lost_ping count = %d after the connection closed, want 1", n) + } +} + +// A health-check ping that fails after the read loop has observed a terminal +// connection error must not be classified as a lost ping, even before +// cleanup marks the connection closed. The read loop is held in that window +// by the test hook. +func TestTransportPingRacingReadLoopExitNotLostPing(t *testing.T) { + synctest.Test(t, testTransportPingRacingReadLoopExitNotLostPing) +} +func testTransportPingRacingReadLoopExitNotLostPing(t *testing.T) { + var lostPings atomic.Int64 + release := make(chan struct{}) + parked := make(chan struct{}) + var parkOnce sync.Once + tc := newTestClientConn(t, func(h2 *http.HTTP2Config) { + h2.PingTimeout = 1 * time.Second + h2.SendPingTimeout = 1 * time.Second + h2.CountError = func(errType string) { + if errType == "conn_close_lost_ping" { + lostPings.Add(1) + } + } + }, func(tr *Transport) { + tr.TestSetReadLoopExitedHook(func(cc *ClientConn) { + parkOnce.Do(func() { close(parked) }) + <-release + }) + }) + tc.greet() + + // The health check fires and its PING is in flight, unanswered. + time.Sleep(1*time.Second + 1*time.Millisecond) + tc.wantFrameType(FramePing) + + // The peer breaks the connection; the read loop observes the terminal + // error and parks in the hook, before cleanup marks the conn closed. + tc.closeWrite() + <-parked + + // The in-flight ping now times out inside that window. + time.Sleep(1 * time.Second) + if n := lostPings.Load(); n != 0 { + t.Errorf("conn_close_lost_ping count = %d for a ping racing the read loop's exit, want 0", n) + } + + close(release) + time.Sleep(2 * time.Second) + if n := lostPings.Load(); n != 0 { + t.Errorf("conn_close_lost_ping count = %d after teardown, want 0", n) + } +} + +// Overlapping health checks must produce exactly one lost-ping count: the +// eligibility check and the close claim share one critical section, so a +// second claimant always observes the first claim. +func TestTransportLostPingClaimedOnce(t *testing.T) { + synctest.Test(t, testTransportLostPingClaimedOnce) +} +func testTransportLostPingClaimedOnce(t *testing.T) { + var lostPings atomic.Int64 + tc := newTestClientConn(t, func(h2 *http.HTTP2Config) { + h2.CountError = func(errType string) { + if errType == "conn_close_lost_ping" { + lostPings.Add(1) + } + } + }) + tc.greet() + + const claimants = 8 + start := make(chan struct{}) + var wg sync.WaitGroup + for range claimants { + wg.Add(1) + go func() { + defer wg.Done() + <-start + tc.cc.TestCloseForLostPing() + }() + } + close(start) + wg.Wait() + if n := lostPings.Load(); n != 1 { + t.Errorf("conn_close_lost_ping count = %d from %d overlapping claims, want 1", n, claimants) + } +} + func TestTransportPingWriteBlocks(t *testing.T) { ts := newTestServer(t, func(w http.ResponseWriter, r *http.Request) {},