http2: fix zombie session crash on socket close#61702
Open
suuuuuuminnnnnn wants to merge 1 commit intonodejs:mainfrom
Open
http2: fix zombie session crash on socket close#61702suuuuuuminnnnnn wants to merge 1 commit intonodejs:mainfrom
suuuuuuminnnnnn wants to merge 1 commit intonodejs:mainfrom
Conversation
Collaborator
|
Review requested:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR fixes a crash caused by HTTP/2 zombie sessions when the underlying socket is dead but the HTTP/2 session remains “alive” in Node.js.
closeevent (e.g. packet drop “black hole”).CHECK(is_write_in_progress())inHttp2Session::OnStreamAfterWriteCHECK(!current_write_)inTLSWrap::DoWritenread < 0) inHttp2Session::OnStreamRead.Fixes: #61304
What is the current behavior?
When the network enters a “black hole” state (packets dropped without RST/FIN), the OS may consider the TCP socket dead/closed, but Node.js can fail to observe the close.
In this case:
session.closed === falsesession.destroyed === falsesession.state.outboundQueueSizeincreases continuously)Relevant code path (before):
What is the new behavior?
On read error (
nread < 0), we still notify the previous listener, then close the HTTP/2 session to prevent zombie state and subsequent assertion crashes.Key points:
Close()is idempotent / safe to call redundantly.Why is this correct?
A negative
nreadindicates an error/EOF condition at the stream/socket read layer. Keeping the HTTP/2 session alive after a read error allows the session to continue queueing outbound frames while the transport is effectively dead, which eventually leads to inconsistent internal write state and assertions.Closing the session on read error restores the expected lifecycle invariant: dead transport ⇒ closed session.
How was this tested?
New test (CI-friendly)
Adds a new parallel test that reproduces the “zombie” behavior without OS firewall rules by destroying the underlying socket directly:
client[kSocket].destroy()to kill the transport.New file:
test/parallel/test-http2-zombie-session-61304.jsCommands
Built locally (macOS arm64):
Ran test:
Spot-checked a related HTTP/2 test:
Additional context / reproduction (original issue)
The original issue can be reproduced reliably on macOS using pf firewall rules to drop packets (reported 100% reproduction), creating a network “black hole” where the socket becomes dead without Node observing a clean close.
Issue: #61304
Files changed
src/node_http2.cc(small change: close session onnread < 0)test/parallel/test-http2-zombie-session-61304.js(new)