-
Notifications
You must be signed in to change notification settings - Fork 30k
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
Comments
@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:
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. |
IIUC, #26962 is a workaround. |
Wow, this is a horrendous limitation. |
@kanongil @CrucialDrew This was fixed in recent release |
I have given up hope that node will ever have professional-grade http2 support, and now pursuing other options. |
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>
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.
The text was updated successfully, but these errors were encountered: