fix(ext/node): handle undefined http2 ping payload and cancel pending pings on destroy#33561
Conversation
fibibot
left a comment
fibibot
left a comment
There was a problem hiding this comment.
LGTM for the targeted scope. The two changes are:
-
payload || Buffer.alloc(8)— bridges the gap between Node'snghttp2_submit_ping(NULL)(sends 8 zero bytes) and Deno's#[buffer]-typed Rust op which requires a typed array. Functional equivalent for the wire bytes. -
pendingPingscancellation incloseSession— invoking each enqueued cb with(false, 0.0, undefined)makes the wrappedpingCallbackdeliverERR_HTTP2_PING_CANCELto the user, which is exactly what the test'sexpectsError({ code: 'ERR_HTTP2_PING_CANCEL' })requires. MirrorsHttp2Session::ClearOutstandingPingsinsrc/node_http2.cc.
Test enrollment alphabetically positioned correctly between options-server-response and pipe.js.
Two notes inline that aren't blocking but worth knowing as you continue working on http2.
CI green on node_compat across all five platforms.
| // destroyed before the PING ACK arrives. Matches Node's | ||
| // ClearOutstandingPings in src/node_http2.cc. | ||
| this[kState].pendingPings.push(cb); | ||
|
|
There was a problem hiding this comment.
Pre-existing issue this PR works around but doesn't fix: the Rust fn ping(&self, #[buffer] payload: &[u8]) in ext/node/ops/http2/session.rs:2365 doesn't take or invoke a callback at all — it just calls nghttp2_submit_ping and returns. So when a real PING ACK arrives via handle_ping_frame, it dispatches to the global ping_frame_cb (onPing in this file, line 573), which only emits a 'ping' event on the session. The user's per-call callback never fires on a successful ACK.
This PR happens to make test-http2-ping-settings-heapdump.js pass because that test only exercises the cancellation path (session.destroy() runs before any ACK could arrive, so the new pendingPings cleanup delivers the expected ERR_HTTP2_PING_CANCEL). A test that pings against a live peer and asserts the cb fires with (null, duration, payload) would still fail.
Not for this PR — flagging so it's tracked. Wiring op_node_http2_ping to thread the cb through (similar to the SETTINGS-ACK queue from #33518) would fix the actual API.
There was a problem hiding this comment.
Agreed — tracking as follow-up. The Rust op needs to mirror the SETTINGS-ACK queue from #33518 so per-call cbs fire on ACK. Out of scope for this PR (cancellation-only test).
|
|
||
| // Track the pending ping so it can be cancelled when the session is | ||
| // destroyed before the PING ACK arrives. Matches Node's | ||
| // ClearOutstandingPings in src/node_http2.cc. |
There was a problem hiding this comment.
pendingPings.push(cb) never removes cb after a successful ACK. If the upstream Rust op were ever fixed to invoke the cb on ACK (see other comment), this loop in closeSession would call the cb a second time on later destroy, surfacing a spurious ERR_HTTP2_PING_CANCEL to the user.
Doesn't manifest today because the cb doesn't fire on ACK at all (the bug above). When the ACK path is fixed, also wrap cb to splice itself out of pendingPings, e.g.:
const pendingPings = this[kState].pendingPings;
const wrapped = (...args) => {
const idx = pendingPings.indexOf(wrapped);
if (idx !== -1) pendingPings.splice(idx, 1);
cb(...args);
};
pendingPings.push(wrapped);
return this[kHandle].ping(buf, wrapped);Or switch to a Set<cb> and delete on ACK.
There was a problem hiding this comment.
Good catch. When the ACK path is wired up (per the other comment), the cb wrapper / Set-and-delete-on-ACK pattern is the right fix. Tracking together as part of the ACK follow-up.
|
@divybot rebase |
… pings on destroy Enables tests/node_compat/runner/suite/test/parallel/test-http2-ping-settings-heapdump.js Co-authored-by: Divy Srivastava <[email protected]>
The previous CI run failed due to specs::task::signals timing out after 45s on the linux-x86_64 release shard. That test exercises the deno task command's signal handling and is unrelated to the http2 polyfill changes in this PR. Co-authored-by: Divy Srivastava <[email protected]>
ae377b4 to
e018a85
Compare
3 participants
Summary
Enables
test-http2-ping-settings-heapdumpin node_compat suite.Test plan
cargo test --test node_compat -- test-http2-ping-settings-heapdump