fix(ext/node): wire http2 maxSettings option to nghttp2_option_set_max_settings#33790
Conversation
…x_settings The `maxSettings` server option (limit on SETTINGS entries per frame, with connection-error response when exceeded) was not wired up. The Rust code was instead routing `maxOutstandingSettings` (a Node.js-internal queue limit for outbound SETTINGS awaiting ACK, with no nghttp2 equivalent) to `nghttp2_option_set_max_settings`, so the server accepted the default 32 entries regardless of the user-supplied `maxSettings`. Read `OptionsIndex::MaxSettings` (index 9) and pass it to nghttp2; drop the wrong call for `MaxOutstandingSettings`. Enables parallel/test-http2-max-settings.js. Co-authored-by: Divy Srivastava <[email protected]>
…x_settings Enables tests/node_compat/runner/suite/test/parallel/test-http2-max-settings.js Co-authored-by: Divy Srivastava <[email protected]>
Co-authored-by: Divy Srivastava <[email protected]>
fibibot
left a comment
fibibot
left a comment
There was a problem hiding this comment.
LGTM. Tight semantic-correction fix. The previous code was conflating two distinct Node.js HTTP/2 options:
maxOutstandingSettings(buffer[7]) — Node-internal queue limit for outbound SETTINGS frames awaiting peer ACK. nghttp2 has no equivalent.maxSettings(buffer[9]) — limit on inbound SETTINGS entries per frame, which is exactly whatnghttp2_option_set_max_settingscontrols.
Old code: nghttp2_option_set_max_settings(options, max_outstanding_settings) — wrong semantics, would silently cap inbound SETTINGS entries to whatever the user passed for maxOutstandingSettings.
New code: nghttp2_option_set_max_settings(options, max_settings) reading from the correct buffer slot, plus an explanatory comment that maxOutstandingSettings is intentionally not mapped to any nghttp2 option (it's a JS-side concern handled via kMaxOutstandingSettings symbol + kState.pendingAck check at http2.ts:3344).
What I checked
- JS side already wires
options.maxSettingstooptionsBuffer[IDX_OPTIONS_MAX_SETTINGS]atinternal/http2/util.ts:283(MathMax(1, options.maxSettings)), so the buffer slot was already populated; the Rust side was just reading the wrong slot. MaxSettings = 9is defined inext/node/ops/http2/types.rs:71, matches the JSIDX_OPTIONS_MAX_SETTINGS = 9. ✓maxOutstandingSettingsJS handling unchanged: still tracked via thekMaxOutstandingSettingssymbol inhttp2.ts:3090,3344and gatespendingAckcount on the polyfill side. The Rust code just stops trying to map it to an nghttp2 option that doesn't exist.
Behavior change to flag
This is a backwards-incompat fix for any user who was relying on the old (buggy) mapping — passing maxOutstandingSettings: N would previously cap nghttp2's inbound-SETTINGS-entries limit to N. After this PR, users need to pass maxSettings: N for that effect. Since the old behavior was semantically wrong (and not what Node does), this is the right direction.
CI
132/135 success, 1 pending (wpt release linux-x86_64 — orthogonal to ext/node http2), 2 skip, 0 fail. The new parallel/test-http2-max-settings.js passes across all platforms.
3 participants
Summary
Enables
test-http2-max-settingsin node_compat suite.Test plan
cargo test --test node_compat -- test-http2-max-settings