Skip to content

Conversation

@ronag
Copy link
Member

@ronag ronag commented Dec 12, 2025

The current defaults are unnecessarily conservative which makes http2 control flow over high latency connections (such as public internet) unbearably slow.

@ronag ronag requested review from jasnell and mcollina December 12, 2025 16:19
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/http2
  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added http2 Issues or PRs related to the http2 subsystem. needs-ci PRs that need a full CI run. labels Dec 12, 2025
@ronag
Copy link
Member Author

ronag commented Dec 12, 2025

I'm unsure about the relationship between request window size (initialWindowSize) and connection window size (connectionWindowSize). But I assume it's ok to have a 1:2 relationship. 🤷

@ronag
Copy link
Member Author

ronag commented Dec 12, 2025

This affects UPLOAD speed, i.e. data to server.

this.emit('session', session);
}

function initializeOptions(options) {
assertIsObject(options, 'options');
options = { ...options };
options = { ...getDefaultSettings(), ...options };
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should actually apply the default settings and not just assume that they are the same as nghttp2 will do.

@ronag ronag force-pushed the http2-tune branch 2 times, most recently from f6732e4 to eae93cc Compare December 12, 2025 16:24
The current defaults are unnecessarily conservative which makes http2 control flow over
high latency connections (such as public internet) unbearably slow.
@codecov
Copy link

codecov bot commented Dec 12, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.47%. Comparing base (d9cf867) to head (f6fa8fa).
⚠️ Report is 352 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #61036      +/-   ##
==========================================
- Coverage   88.58%   88.47%   -0.12%     
==========================================
  Files         704      703       -1     
  Lines      207815   208549     +734     
  Branches    40036    40199     +163     
==========================================
+ Hits       184102   184517     +415     
- Misses      15758    16027     +269     
- Partials     7955     8005      +50     
Files with missing lines Coverage Δ
lib/internal/http2/core.js 93.85% <100.00%> (-1.36%) ⬇️
lib/internal/http2/util.js 92.74% <100.00%> (+0.05%) ⬆️

... and 145 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

I think this might require a doc change too

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

http2 Issues or PRs related to the http2 subsystem. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants