-
Notifications
You must be signed in to change notification settings - Fork 675
Fix: send connection-level WINDOW_UPDATE at session start #2971
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
base: master
Are you sure you want to change the base?
Fix: send connection-level WINDOW_UPDATE at session start #2971
Conversation
If the user sets `grpc-node.connection_flow_control_window` to a value > 65 535 B, we now send a WINDOW_UPDATE (or setLocalWindowSize) right after `http2.connect()` returns. This removes the 65 KB start-window stall that caused large initial backlogs on high-throughput streams, especially when an H2 proxy (e.g. HAProxy) sat between client and server. Behaviour now matches Go/Rust gRPC; no API changes.
….flow_control_window
The |
I’ve moved it to remoteSettings, and it still works as expected 🚀. Mind giving it another review? |
@murgatroid99 When you have a moment, could you take another look and let me know if it’s good to go? |
any updates here? We need the same fix |
@@ -722,11 +722,30 @@ export class Http2SubchannelConnector implements SubchannelConnector { | |||
http2.getDefaultSettings().initialWindowSize, | |||
} | |||
}); | |||
|
|||
// Prepare window size configuration for remoteSettings handler | |||
const defaultWin = http2.getDefaultSettings().initialWindowSize ?? 65535; // 65 535 B |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In what situation would http2.getDefaultSettings().initialWindowSize
not have a value? Either way, this line should be consistent with the equivalent usage on line 722: either both should have a hardcoded default value, or neither.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@KoenRijpstra are you planning on addressing this feedback? If you don't have time I can do it for you
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes will address this today.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@murgatroid99 The ?? 65535
fallback is purely for TypeScript compliance and defensive programming. In practice, http2.getDefaultSettings().initialWindowSize
will always be 65535
, but the fallback ensures type safety and makes the code more robust.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@murgatroid99 Both now have the hardcoded value.
Overview
@grpc/grpc-js
previously opened every HTTP/2 connection with the RFC‑default 65 535‑byte connection window, then waited until after the first DATA frames were received before enlarging it. On fast, high‑volume streams this small initial window causes the sender (or any intermediate HTTP/2 proxy) to buffer excess data, creating avoidable latency.This change immediately enlarges the connection‑level window as soon as the session is created when the application has requested a larger value via
'grpc-node.flow_control_window'
This aligns the Node implementation with other gRPC language stacks that send the larger window before any payload is transmitted.
Core change (single patch)
No public API changes. Clients that do not set the option keep the existing behaviour.
Impact
Works on Node 14 → 20 (fallback path covers older Node versions).
Changelog entry