Skip to content

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.

Sign up for GitHub

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
+20 −1
Open

Fix: send connection-level WINDOW_UPDATE at session start #2971

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d872278
fix(http2): bump connection window immediately after connect()
KoenRijpstra Jun 27, 2025
aeb7a5f
fix(http2): handle default initial window size correctly
KoenRijpstra Jun 27, 2025
b69bcad
fix(http2): rename connection flow control window option to grpc-node…
KoenRijpstra Jun 27, 2025
0e09b9c
fix(http2): move WINDOW_UPDATE handling to remoteSettings event
KoenRijpstra Jun 27, 2025
a8d22dc
fix(http2): set default initial window size to 65535 if not specified
KoenRijpstra Aug 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 20 additions & 1 deletion packages/grpc-js/src/transport.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -719,14 +719,33 @@ export class Http2SubchannelConnector implements SubchannelConnector {
settings: {
initialWindowSize:
options['grpc-node.flow_control_window'] ??
http2.getDefaultSettings().initialWindowSize,
http2.getDefaultSettings().initialWindowSize ?? 65535,
}
});

// Prepare window size configuration for remoteSettings handler
const defaultWin = http2.getDefaultSettings().initialWindowSize ?? 65535; // 65 535 B
Copy link
Member

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.

Copy link

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

Copy link
Author

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.

Copy link
Author

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.

Copy link
Author

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.

const connWin = options[
'grpc-node.flow_control_window'
] as number | undefined;

this.session = session;
let errorMessage = 'Failed to connect';
let reportedError = false;
session.unref();
session.once('remoteSettings', () => {
// Send WINDOW_UPDATE now to avoid 65 KB start-window stall.
if (connWin && connWin > defaultWin) {
try {
// Node ≥ 14.18
(session as any).setLocalWindowSize(connWin);
} catch {
// Older Node: bump by the delta
const delta = connWin - (session.state.localWindowSize ?? defaultWin);
if (delta > 0) (session as any).incrementWindowSize(delta);
}
}

session.removeAllListeners();
secureConnectResult.socket.removeListener('close', closeHandler);
secureConnectResult.socket.removeListener('error', errorHandler);
Expand Down