Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

http2 - allow option for setting the local window size of a session #31084

Closed
CrucialDrew opened this issue Dec 24, 2019 · 6 comments
Closed
Labels
http2 Issues or PRs related to the http2 subsystem.

Comments

@CrucialDrew
Copy link

The problem

The current http2 implementation only allows the SETTINGS_INITIAL_WINDOW_SIZE to be set. This is the flow-control for the stream within the session, but not for the entire session. The default value for the session and window is 65,535. The node implementation of nghttp2 only allows the setting of the stream window size, meaning the entire session can never exceed the throughput of a 64K window.

On as little as 20ms of latency, this limits the entire session to 3MB/s. SETTINGS_INITIAL_WINDOW_SIZE, the only setting exposed for flow control (https://nodejs.org/api/http2.html#http2_settings_object, initialWindowSize), would never allow you to exceed the session window. This means, all you're really allowed to do, is further limit individual streams below that default 64K session window.

The solution

In node's current dependency library, nghttp2 includes a way of increasing this limit properly. It works fine with Google Chrome as a client and nginx as a server. There's no obvious reason it would not work for all clients and all servers.

The call is nghttp2_session_set_local_window_size

I don't have experience with writing node modules, but I can safely increase this limit in the following function:

void Http2Session::New(const FunctionCallbackInfo& args)

... by appending the following line to that function:

nghttp2_session_set_local_window_size(session->session(), NGHTTP2_FLAG_NONE, 0, 64 * 1024 * 1024);

This increases the session window from 64K to 64M.

I'd suggest this be an option you can pass in node, since there is value in being able to set the session window based on your needs for flow control. As far as I understand it, this can also be set dynamically after the session creation with window updates. It would be even better if you could change this value as a reaction to usage.

@addaleax addaleax added the http2 Issues or PRs related to the http2 subsystem. label Dec 24, 2019
@addaleax
Copy link
Member

addaleax commented Dec 24, 2019

@CrucialDrew Since you already looked into this quite a bit, would you maybe be interested in contributing this feature yourself?

@CrucialDrew
Copy link
Author

@CrucialDrew Since you already looked into this quite a bit, would you maybe be interested in contributing this feature yourself?

I just don't think I have enough experience working on node modules to do so effectively.

I think it would be best suited as an option here: https://nodejs.org/api/http2.html#http2_settings_object (maybe sessionLocalWindowSize)

... but then these things would all have to hook into it:

  • http2.connect(authority[, options][, listener])
  • http2.createServer()
  • http2.createSecureServer()
  • http2session.settings

I have a feeling an experienced node module developer could implement that out a lot faster than me, since this was the first time I've looked at node module code.

@ZYSzys
Copy link
Member

ZYSzys commented Jan 4, 2020

IIUC, #26962 is a workaround.

@kanongil
Copy link
Contributor

kanongil commented Nov 4, 2020

Wow, this is a horrendous limitation.

@ZYSzys ZYSzys closed this as completed in c0ac692 Nov 11, 2020
codebytere pushed a commit that referenced this issue Nov 22, 2020
PR-URL: #35978
Fixes: #31084
Refs: #26962
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ricky Zhou <0x19951125@gmail.com>
@ZYSzys
Copy link
Member

ZYSzys commented Dec 1, 2020

@kanongil @CrucialDrew This was fixed in recent release v15.3.0(in #35978), can you try to confirm if that PR fixed your issue ?

@kanongil
Copy link
Contributor

kanongil commented Dec 1, 2020

I have given up hope that node will ever have professional-grade http2 support, and now pursuing other options.

targos pushed a commit that referenced this issue Aug 4, 2021
PR-URL: #35978
Fixes: #31084
Refs: #26962
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ricky Zhou <0x19951125@gmail.com>
targos pushed a commit that referenced this issue Aug 4, 2021
PR-URL: #35978
Fixes: #31084
Refs: #26962
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ricky Zhou <0x19951125@gmail.com>
BethGriggs pushed a commit that referenced this issue Aug 12, 2021
PR-URL: #35978
Fixes: #31084
Refs: #26962
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ricky Zhou <0x19951125@gmail.com>
MylesBorins pushed a commit that referenced this issue Aug 31, 2021
PR-URL: #35978
Fixes: #31084
Refs: #26962
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ricky Zhou <0x19951125@gmail.com>
foxxyz pushed a commit to foxxyz/node that referenced this issue Oct 18, 2021
PR-URL: nodejs#35978
Fixes: nodejs#31084
Refs: nodejs#26962
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ricky Zhou <0x19951125@gmail.com>
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.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants