-
-
Notifications
You must be signed in to change notification settings - Fork 687
feat(client): expose HTTP/2 flow-control options #4706
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: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,101 @@ | ||
| 'use strict' | ||
|
|
||
| const { tspl } = require('@matteo.collina/tspl') | ||
| const { test, after } = require('node:test') | ||
| const { EventEmitter } = require('node:events') | ||
| const connectH2 = require('../lib/dispatcher/client-h2') | ||
| const { | ||
| kUrl, | ||
| kSocket, | ||
| kMaxConcurrentStreams, | ||
| kHTTP2Session, | ||
| kHTTP2InitialWindowSize, | ||
| kHTTP2ConnectionWindowSize | ||
| } = require('../lib/core/symbols') | ||
|
|
||
| test('Should plumb http2.initialWindowSize and http2.connectionWindowSize into the HTTP/2 session creation path', async (t) => { | ||
| t = tspl(t, { plan: 6 }) | ||
|
|
||
| const http2 = require('node:http2') | ||
| const originalConnect = http2.connect | ||
|
|
||
| /** @type {any} */ | ||
| let seenConnectOptions = null | ||
| /** @type {number[]} */ | ||
| const setLocalWindowSizeCalls = [] | ||
|
|
||
| class FakeSession extends EventEmitter { | ||
| unref () {} | ||
| ref () {} | ||
| close () {} | ||
| destroy () {} | ||
| request () { | ||
| throw new Error('not implemented') | ||
| } | ||
|
|
||
| setLocalWindowSize (size) { | ||
| setLocalWindowSizeCalls.push(size) | ||
| } | ||
| } | ||
|
|
||
| class FakeSocket extends EventEmitter { | ||
| constructor () { | ||
| super() | ||
| this.destroyed = false | ||
| } | ||
|
|
||
| unref () {} | ||
| ref () {} | ||
| destroy () { | ||
| this.destroyed = true | ||
| return this | ||
| } | ||
| } | ||
|
|
||
| const fakeSession = new FakeSession() | ||
|
|
||
| http2.connect = function connectStub (_authority, options) { | ||
| seenConnectOptions = options | ||
| return fakeSession | ||
| } | ||
|
|
||
| after(() => { | ||
| http2.connect = originalConnect | ||
| }) | ||
|
|
||
| const initialWindowSize = 12345 | ||
| const connectionWindowSize = 77777 | ||
|
|
||
| const client = { | ||
| [kUrl]: new URL('https://localhost'), | ||
| [kMaxConcurrentStreams]: 100, | ||
| [kHTTP2InitialWindowSize]: initialWindowSize, | ||
| [kHTTP2ConnectionWindowSize]: connectionWindowSize, | ||
| [kSocket]: null, | ||
| [kHTTP2Session]: null | ||
| } | ||
|
|
||
| const socket = new FakeSocket() | ||
|
|
||
| connectH2(client, socket) | ||
|
|
||
| t.ok(seenConnectOptions && seenConnectOptions.settings) | ||
| t.strictEqual(seenConnectOptions.settings.enablePush, false) | ||
| t.strictEqual( | ||
| seenConnectOptions.settings.initialWindowSize, | ||
| initialWindowSize | ||
| ) | ||
| t.strictEqual(client[kHTTP2Session], fakeSession) | ||
|
|
||
| // Emit 'connect' event | ||
| process.nextTick(() => { | ||
| fakeSession.emit('connect') | ||
| }) | ||
|
|
||
| await new Promise((resolve) => process.nextTick(resolve)) | ||
|
|
||
| t.strictEqual(setLocalWindowSizeCalls.length, 1) | ||
| t.strictEqual(setLocalWindowSizeCalls[0], connectionWindowSize) | ||
|
|
||
| await t.completed | ||
| }) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -92,6 +92,21 @@ export declare namespace Client { | |
| * @default 100 | ||
| */ | ||
| maxConcurrentStreams?: number; | ||
| /** | ||
| * @description HTTP/2 session options. | ||
| */ | ||
| http2?: { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the undici team probably prefer to avoid the
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, remove the |
||
| /** | ||
| * @description Sets the HTTP/2 stream-level flow-control window size (SETTINGS_INITIAL_WINDOW_SIZE). | ||
| * @default undefined | ||
| */ | ||
| initialWindowSize?: number; | ||
| /** | ||
| * @description Sets the HTTP/2 connection-level flow-control window size (ClientHttp2Session.setLocalWindowSize). | ||
| * @default undefined | ||
| */ | ||
| connectionWindowSize?: number; | ||
| }; | ||
| } | ||
| export interface SocketInfo { | ||
| localAddress?: string | ||
|
|
||
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.
I think we can have slightly better defaults than Node.js core for these.
See @ronag in nodejs/node#61036