net/http: count a lost HTTP/2 ping exactly once, and never on a failed connection

src/net/http/internal/http2/clientconn_test.go

@@ -562,13 +562,20 @@ 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)
 		}
 	}
 	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)

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 }
 

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
 		}

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) {},