Skip to content

http2: count a lost ping exactly once, and never on a failed connection - #262

Open
ali-sayyah wants to merge 1 commit into
golang:masterfrom
ali-sayyah:http2-close-healthcheck
Open

http2: count a lost ping exactly once, and never on a failed connection#262
ali-sayyah wants to merge 1 commit into
golang:masterfrom
ali-sayyah:http2-close-healthcheck

Conversation

@ali-sayyah

@ali-sayyah ali-sayyah commented Aug 17, 2026

Copy link
Copy Markdown

The health-check timer is re-armed after every ReadFrame return,
including the final erroring one, and no close path stops it: one
ReadIdleTimeout 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.

CL 198040 shipped the health check with the timer stopped when the
read loop exits; CL 572378, an unrelated test-infrastructure
conversion, dropped the stop with no behavioral rationale (v0.22.0
has it, v0.23.0 does not). Restore the stop, 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 the write-blocked-ping detection of CL 354389.

This fixes the legacy transport, used on Go versions before 1.27 and
with the http2legacy build tag (go build -tags=http2legacy). The
default Go 1.27 path uses net/http's copy of this code, which has the
same defect and is fixed separately; that change closes the issue.

Updates golang/go#80920

@ali-sayyah
ali-sayyah force-pushed the http2-close-healthcheck branch from 75cdb10 to 5d6dfed Compare August 17, 2026 19:24
@ali-sayyah ali-sayyah changed the title http2: don't health-check connections after the read loop exits http2: don't count lost pings on terminally failed connections Aug 17, 2026
The health-check timer is re-armed after every ReadFrame return,
including the final erroring one, and no close path stops it: one
ReadIdleTimeout 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.

CL 198040 shipped the health check with the timer stopped when the
read loop exits; CL 572378, an unrelated test-infrastructure
conversion, dropped the stop with no behavioral rationale (v0.22.0
has it, v0.23.0 does not). Restore the stop, 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 the write-blocked-ping detection of CL 354389.

This fixes the legacy transport, used on Go versions before 1.27 and
with the http2legacy build tag (go build -tags=http2legacy). The
default Go 1.27 path uses net/http's copy of this code, which has the
same defect and is fixed separately; that change closes the issue.

Updates golang/go#80920
@ali-sayyah
ali-sayyah force-pushed the http2-close-healthcheck branch from 5d6dfed to 8261ddc Compare August 17, 2026 20:07
@ali-sayyah ali-sayyah changed the title http2: don't count lost pings on terminally failed connections http2: count a lost ping exactly once, and never on a failed connection Aug 17, 2026
ali-sayyah added a commit to ali-sayyah/go that referenced this pull request Aug 17, 2026
…d 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 golang#80920
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant