Uh oh!
There was an error while loading. Please reload this page.
http2: increase default window sizes - #64623
Conversation
nodejs-github-bot
commented
Jul 20, 2026
Review requested:
|
| // to window_size / RTT. With a 32MB connection window, throughput is | ||
| // significantly improved. See https://github.com/nodejs/node/issues/38426 | ||
| CHECK_EQ(nghttp2_session_set_local_window_size( | ||
| session, NGHTTP2_FLAG_NONE, 0, 33554432), 0); |
There was a problem hiding this comment.
There's a pre-existing issue that this exposes: you can't configure this in advance. You can only control the connection window with session.setLocalWindowSize once you have a session, but at that point this initial WINDOW_UPDATE has already been sent, and shrinking the window isn't effective as shown in the tests here (it just blocks further updates, it can't reduce what's already been sent). The stream window is configurable in advance (it's a SETTINGS parameter) but this connection window isn't.
That already existed implicitly but it was kind of OK when the initial window was small, but it's a real problem with a very large initial window I think, since there's basically no way to override this to make it any smaller again if you need to.
Can we add client & server options for this, so it can be configured before the first WINDOW_UPDATE is sent somehow?
| // to window_size / RTT. With a 32MB connection window, throughput is | ||
| // significantly improved. See https://github.com/nodejs/node/issues/38426 | ||
| CHECK_EQ(nghttp2_session_set_local_window_size( | ||
| session, NGHTTP2_FLAG_NONE, 0, 33554432), 0); |
There was a problem hiding this comment.
Nit: would be nicer to extract this as a constant
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@## main #64623 +/- ##
==========================================
- Coverage 90.16% 90.16% -0.01%
==========================================
Files 746 746 Lines 242760 242773 +13 Branches 45765 45766 +1 ==========================================
+ Hits 218875 218885 +10 + Misses 15375 15368 -7 - Partials 8510 8520 +10
🚀 New features to boost your workflow:
|
5c9bc03 to
4351b51Comparemcollina
commented
Jul 20, 2026
Both addressed:
|
4351b51 to
31761a0Compare
RafaelGSS
left a comment
There was a problem hiding this comment.
Wouldn't it be
semver-major
mcollina
commented
Jul 20, 2026
yes |
70882db to
bd4cbd3Comparepimterry
commented
Jul 21, 2026
I do think this would be useful, but doesn't need to block this - I'll open a separate PR. |
bd4cbd3 to
86b969bCompare86b969b to
d0c68baCompare
Ethan-Arrowood
left a comment
There was a problem hiding this comment.
CI failure seems related to the change here; test-http2-respond-with-file-connection-abort is failing across multiple systems and that isn't a known flake. Leaving a request changes to this gets fixed before landing.
When a stream is destroyed while a synchronous write is in flight from StreamPipe::ProcessData, ReportWritesToJSStreamListener:: OnStreamAfterReqFinished can be called with a null stream_ or a null req_wrap, causing a null pointer dereference. This race becomes more likely with larger HTTP/2 windows (e.g., nodejs#38426) because more data is in flight during session teardown. Add early-return guards for both cases to prevent the crash. Refs: nodejs#38426 Refs: nodejs#56825 Signed-off-by: Matteo Collina <hello@matteocollina.com>
With the default window size increase, the server sends an additional WINDOW_UPDATE frame during session setup, increasing the frames received by the client from 7 to 8 on some platforms. Relax the assertion to check >= 7 instead of strict equality. Refs: nodejs#38426 Signed-off-by: Matteo Collina <hello@matteocollina.com>
When the writable sink is destroyed while there are still pending writes, WritableListener::OnStreamDestroy previously reset pending_writes_ to 0. This caused OnStreamAfterWrite to decrement the counter below 0, making the pending_writes_ == 0 completion check fail permanently — the oncomplete callback was never called, causing a timeout. Fix by: 1. Not resetting pending_writes_ in OnStreamDestroy — let OnStreamAfterWrite track completion naturally. 2. Using sink_destroyed_ flag in Unpipe to always remove the writable listener regardless of pending_writes_ count. 3. Using <= 0 (instead of == 0) when sink_destroyed_ is set, and guarding RemoveStreamListener against a null stream. This race became more likely with larger HTTP/2 windows (nodejs#38426) because more writes are in flight when session teardown begins. Refs: nodejs#38426 Refs: nodejs#56825 Signed-off-by: Matteo Collina <hello@matteocollina.com>
Signed-off-by: Matteo Collina <hello@matteocollina.com>
a25a676 to
f399373Compare
This comment was marked as outdated.
This comment was marked as outdated.
nodejs-github-bot
commented
Aug 3, 2026
nodejs-github-bot
commented
Aug 3, 2026
nodejs-github-bot
commented
Aug 3, 2026
Commit Queue failed- Loading data for nodejs/node/pull/64623 ✔ Done loading data for nodejs/node/pull/64623 ----------------------------------- PR info ------------------------------------ Title http2: increase default window sizes (#64623) Author Matteo Collina <matteo.collina@gmail.com> (@mcollina) Branch mcollina:increase-http2-window-size -> nodejs:main Labels c++, semver-major, lib / src Commits 5 - http2: increase default window sizes - http2: guard against null stream/req in OnStreamAfterReqFinished - http2: relax framesReceived assertion in perf_hooks test - stream: fix pending_writes underflow in StreamPipe shutdown - style: fix clang-format alignment in CHECK_EQ call Committers 1 - Matteo Collina <hello@matteocollina.com> PR-URL: https://github.com/nodejs/node/pull/64623 Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Tim Perry <pimterry@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gürgün Dayıoğlu <hey@gurgun.day> Reviewed-By: Ethan Arrowood <ethan@arrowood.dev> Reviewed-By: Aviv Keller <me@aviv.sh> Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/64623 Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Tim Perry <pimterry@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gürgün Dayıoğlu <hey@gurgun.day> Reviewed-By: Ethan Arrowood <ethan@arrowood.dev> Reviewed-By: Aviv Keller <me@aviv.sh> Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> -------------------------------------------------------------------------------- ℹ This PR was created on Mon, 20 Jul 2026 12:33:00 GMT ✔ Approvals: 8 ✔ - Rafael Gonzaga (@RafaelGSS) (TSC): https://github.com/nodejs/node/pull/64623#pullrequestreview-4737957628 ✔ - Tim Perry (@pimterry): https://github.com/nodejs/node/pull/64623#pullrequestreview-4765202215 ✔ - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/64623#pullrequestreview-4749623762 ✔ - Gürgün Dayıoğlu (@gurgunday): https://github.com/nodejs/node/pull/64623#pullrequestreview-4765616109 ✔ - Ethan Arrowood (@Ethan-Arrowood): https://github.com/nodejs/node/pull/64623#pullrequestreview-4767534645 ✔ - Aviv Keller (@avivkeller): https://github.com/nodejs/node/pull/64623#pullrequestreview-4770440465 ✔ - Robert Nagy (@ronag) (TSC): https://github.com/nodejs/node/pull/64623#pullrequestreview-4838488304 ✔ - Trivikram Kamat (@trivikr): https://github.com/nodejs/node/pull/64623#pullrequestreview-4841424027 ✔ Last GitHub CI successful ℹ Last Full PR CI on 2026-08-03T12:22:08Z: https://ci.nodejs.org/job/node-test-pull-request/75428/ - Querying data for job/node-test-pull-request/75428/ ✔ Build data downloaded ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ No git cherry-pick in progress ✔ No git am in progress ✔ No git rebase in progress -------------------------------------------------------------------------------- - Bringing origin/main up to date... From https://github.com/nodejs/node * branch main -> FETCH_HEAD ✔ origin/main is now up-to-date - Downloading patch for 64623 From https://github.com/nodejs/node * branch refs/pull/64623/merge -> FETCH_HEAD ✔ Fetched commits as 7e439828de88..f3993733daf8 -------------------------------------------------------------------------------- Auto-merging doc/api/http2.md [main d458cadee7] http2: increase default window sizes Author: Matteo Collina <hello@matteocollina.com> Date: Mon Jul 20 12:09:08 2026 +0000 16 files changed, 78 insertions(+), 61 deletions(-) [main 2e28518bd9] http2: guard against null stream/req in OnStreamAfterReqFinished Author: Matteo Collina <hello@matteocollina.com> Date: Thu Jul 23 14:28:24 2026 +0000 1 file changed, 2 insertions(+) [main 8974b46a94] http2: relax framesReceived assertion in perf_hooks test Author: Matteo Collina <hello@matteocollina.com> Date: Fri Jul 24 08:19:42 2026 +0000 1 file changed, 5 insertions(+), 1 deletion(-) [main 83d3ac6cb0] stream: fix pending_writes underflow in StreamPipe shutdown Author: Matteo Collina <hello@matteocollina.com> Date: Fri Jul 24 11:03:27 2026 +0000 1 file changed, 13 insertions(+), 4 deletions(-) [main 93ebe33adb] style: fix clang-format alignment in CHECK_EQ call Author: Matteo Collina <hello@matteocollina.com> Date: Fri Jul 24 15:10:46 2026 +0000 1 file changed, 3 insertions(+), 5 deletions(-) ✔ Patches applied There are 5 commits in the PR. Attempting autorebase. (node:365) [DEP0190] DeprecationWarning: Passing args to a child process with shell option true can lead to security vulnerabilities, as the arguments are not escaped, only concatenated. (Use `node --trace-deprecation ...` to show where the warning was created) Rebasing (2/10) Executing: git node land --amend --yes --------------------------------- New Message ---------------------------------- http2: increase default window sizeshttps://github.com/nodejs/node/actions/runs/30825144898 |
trivikr
commented
Aug 3, 2026
@mcollina This either needs |
Uh oh!
There was an error while loading. Please reload this page.
nodejs-github-bot
commented
Aug 4, 2026
Landed in 1576cb8 |
Increase the default HTTP/2 stream window from 64KB (65535) to 4MB (4194304) and the default local connection window to 32MB (33554432). The default 64KB window limits throughput on high-latency connections to window_size / RTT. With a 250ms RTT, throughput is limited to 256KB/s. The new defaults improve throughput to 16MB/s (128Mbps) for the stream window and 128MB/s (1Gbps) for the connection window. Fixes: #38426 Signed-off-by: Matteo Collina <hello@matteocollina.com> PR-URL: #64623 Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Tim Perry <pimterry@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gürgün Dayıoğlu <hey@gurgun.day> Reviewed-By: Ethan Arrowood <ethan@arrowood.dev> Reviewed-By: Aviv Keller <me@aviv.sh> Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Increase the default HTTP/2 stream window from 64KB (65535) to 4MB
(4194304) and the default local connection window to 32MB (33554432).
The default 64KB window limits throughput on high-latency connections
to
window_size / RTT. With a 250ms RTT, throughput is limited to256KB/s. The new defaults improve throughput to 16MB/s (128Mbps)
for the stream window and 128MB/s (1Gbps) for the connection window.
Fixes#38426