fix(ext/node): don't fire http2 settings callback after session destroy#36230
Open
crowlbot wants to merge 2 commits into
Open
fix(ext/node): don't fire http2 settings callback after session destroy#36230crowlbot wants to merge 2 commits into
crowlbot wants to merge 2 commits into
Conversation
The http2 settings ACK callback (settingsCallback) was registered directly on the session handle with no guard against the session already being destroyed. Unlike pending PINGs — which closeSession() cancels atomically on destroy — a SETTINGS_ACK that arrives in the window after session.destroy() would still invoke the user's settings callback, because the handle's ACK dispatch isn't guaranteed to be torn down synchronously with destroy(). This matches Node's Http2Session::Close (src/node_http2.cc), which detaches the pending Http2Settings (dropping its callback) rather than running it. Guard settingsCallback so a destroyed session never invokes the user callback. Fixes the intermittent failure of the mustNotCall() settings callback in node_compat parallel/test-http2-ping-settings-heapdump.js on slower runners (seen on macos-aarch64). Closes #36141
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
1 participant
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.
Fixes #36141.
node_compat::parallel::test-http2-ping-settings-heapdump.jsfails intermittently onmacos-aarch64, recurring across independent commits and surviving the runner's built-in flaky-retry.Root cause. The test (a regression test for nodejs/node#28088) runs two variants. For the
settingsvariant it does:i.e. after destroying the session, the pending settings ACK callback must not fire. In this polyfill the two acknowledgement paths are asymmetric:
state.pendingPingsand cancelled atomically incloseSession()on destroy (the array is emptied, so a late PONG delivered toonPingfinds nothing). Deterministic.settingsCallback) are bound directly onto the session handle with no destroy guard. The handle's SETTINGS_ACK dispatch isn't guaranteed to be torn down synchronously withsession.destroy(), so a SETTINGS_ACK arriving in the window right after destroy still invokes the user callback — trippingmustNotCall(). Whether the ACK or the teardown wins is timing-dependent, which is why it only shows up on slower runners.Fix. Guard
settingsCallbackso a destroyed session never invokes the user callback. This mirrors Node'sHttp2Session::Close(src/node_http2.cc), which detaches the pendingHttp2Settings(dropping its callback) rather than running it, and makes the settings path deterministic on destroy just like the ping path already is.The change is defensive and Node-matching: on a live (non-destroyed) session, legitimate settings ACKs still fire as before.