From e018dccc0c3540927477ca03aea6c9f60ed609a8 Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Thu, 26 Jun 2025 12:16:43 +0200 Subject: [PATCH 1/3] feat(#4230): Implement pingInterval for dispatching PING frames --- docs/docs/api/Client.md | 1 + docs/docs/api/H2CClient.md | 1 + lib/core/symbols.js | 1 + lib/dispatcher/client-h2.js | 43 +++++- lib/dispatcher/client.js | 14 +- test/http2.js | 267 +++++++++++++++++++++++++++++++++++- types/client.d.ts | 5 + 7 files changed, 322 insertions(+), 10 deletions(-) diff --git a/docs/docs/api/Client.md b/docs/docs/api/Client.md index eab6ddc45ac..a915c4a90dc 100644 --- a/docs/docs/api/Client.md +++ b/docs/docs/api/Client.md @@ -31,6 +31,7 @@ Returns: `Client` * **autoSelectFamilyAttemptTimeout**: `number` - Default: depends on local Node version, on Node 18.13.0 and above is `250`. The amount of time in milliseconds to wait for a connection attempt to finish before trying the next address when using the `autoSelectFamily` option. See [here](https://nodejs.org/api/net.html#socketconnectoptions-connectlistener) for more details. * **allowH2**: `boolean` - Default: `false`. Enables support for H2 if the server has assigned bigger priority to it through ALPN negotiation. * **maxConcurrentStreams**: `number` - Default: `100`. Dictates the maximum number of concurrent streams for a single H2 session. It can be overridden by a SETTINGS remote frame. +* **pingInterval**: `number` - Default: `60e3`. The time interval in milliseconds between PING frames sent to the server. Set to `0` to disable PING frames. This is only applicable for HTTP/2 connections. > **Notes about HTTP/2** > - It only works under TLS connections. h2c is not supported. diff --git a/docs/docs/api/H2CClient.md b/docs/docs/api/H2CClient.md index c9bbb3f17e4..1f1a8be4562 100644 --- a/docs/docs/api/H2CClient.md +++ b/docs/docs/api/H2CClient.md @@ -48,6 +48,7 @@ Returns: `H2CClient` - **maxResponseSize** `number | null` (optional) - Default: `-1` - The maximum length of response body in bytes. Set to `-1` to disable. - **maxConcurrentStreams**: `number` - Default: `100`. Dictates the maximum number of concurrent streams for a single H2 session. It can be overridden by a SETTINGS remote frame. - **pipelining** `number | null` (optional) - Default to `maxConcurrentStreams` - The amount of concurrent requests sent over a single HTTP/2 session in accordance with [RFC-7540](https://httpwg.org/specs/rfc7540.html#StreamsLayer) Stream specification. Streams can be closed up by remote server at any time. +- **pingInterval**: `number` - Default: `60e3`. The time interval in milliseconds between PING frames sent to the server. Set to `0` to disable PING frames. This is only applicable for HTTP/2 connections. - **connect** `ConnectOptions | null` (optional) - Default: `null`. - **strictContentLength** `Boolean` (optional) - Default: `true` - Whether to treat request content length mismatches as errors. If true, an error is thrown when the request content-length header doesn't match the length of the request body. - **autoSelectFamily**: `boolean` (optional) - Default: depends on local Node version, on Node 18.13.0 and above is `false`. Enables a family autodetection algorithm that loosely implements section 5 of [RFC 8305](https://tools.ietf.org/html/rfc8305#section-5). See [here](https://nodejs.org/api/net.html#socketconnectoptions-connectlistener) for more details. This option is ignored if not supported by the current Node version. diff --git a/lib/core/symbols.js b/lib/core/symbols.js index f3b563a5419..fa208bc4fb2 100644 --- a/lib/core/symbols.js +++ b/lib/core/symbols.js @@ -62,6 +62,7 @@ module.exports = { kListeners: Symbol('listeners'), kHTTPContext: Symbol('http context'), kMaxConcurrentStreams: Symbol('max concurrent streams'), + kPingInterval: Symbol('ping interval'), kNoProxyAgent: Symbol('no proxy agent'), kHttpProxyAgent: Symbol('http proxy agent'), kHttpsProxyAgent: Symbol('https proxy agent') diff --git a/lib/dispatcher/client-h2.js b/lib/dispatcher/client-h2.js index 661d857bee1..23a73533613 100644 --- a/lib/dispatcher/client-h2.js +++ b/lib/dispatcher/client-h2.js @@ -23,12 +23,14 @@ const { kStrictContentLength, kOnError, kMaxConcurrentStreams, + kPingInterval, kHTTP2Session, kResume, kSize, kHTTPContext, kClosed, - kBodyTimeout + kBodyTimeout, + kHTTP2SessionState } = require('../core/symbols.js') const { channels } = require('../core/diagnostics.js') @@ -78,8 +80,6 @@ function parseH2Headers (headers) { } async function connectH2 (client, socket) { - client[kSocket] = socket - const session = http2.connect(client[kUrl], { createConnection: () => socket, peerMaxConcurrentStreams: client[kMaxConcurrentStreams], @@ -89,10 +89,15 @@ async function connectH2 (client, socket) { } }) + client[kSocket] = socket session[kOpenStreams] = 0 session[kClient] = client session[kSocket] = socket - session[kHTTP2Session] = null + session[kHTTP2SessionState] = { + ping: { + interval: client[kPingInterval] === 0 ? null : setInterval(onHttp2SendPing, client[kPingInterval], session) + } + } util.addListener(session, 'error', onHttp2SessionError) util.addListener(session, 'frameError', onHttp2FrameError) @@ -151,6 +156,29 @@ function resumeH2 (client) { } } +function onHttp2SendPing (session) { + const state = session[kHTTP2SessionState] + if ((session.closed || session.destroyed) && state.ping.interval != null) { + clearInterval(state.ping.interval) + state.ping.interval = null + return + } + + // If no ping sent, do nothing + session.ping(onPing.bind(session)) + + function onPing (err, duration) { + const client = this[kClient] + + if (err != null) { + // TODO: should we drop the session? + client[kOnError](err) + } else { + client.emit('ping', duration) + } + } +} + function onHttp2SessionError (err) { assert(err.code !== 'ERR_TLS_CERT_ALTNAME_INVALID') @@ -214,7 +242,7 @@ function onHttp2SessionGoAway (errorCode) { } function onHttp2SessionClose () { - const { [kClient]: client } = this + const { [kClient]: client, [kHTTP2SessionState]: state } = this const { [kSocket]: socket } = client const err = this[kSocket][kError] || this[kError] || new SocketError('closed', util.getSocketInfo(socket)) @@ -222,6 +250,11 @@ function onHttp2SessionClose () { client[kSocket] = null client[kHTTPContext] = null + if (state.ping.interval != null) { + clearInterval(state.ping.interval) + state.ping.interval = null + } + if (client.destroyed) { assert(client[kPending] === 0) diff --git a/lib/dispatcher/client.js b/lib/dispatcher/client.js index 0b0990206e7..b38bbea0eff 100644 --- a/lib/dispatcher/client.js +++ b/lib/dispatcher/client.js @@ -52,7 +52,8 @@ const { kOnError, kHTTPContext, kMaxConcurrentStreams, - kResume + kResume, + kPingInterval } = require('../core/symbols.js') const connectH1 = require('./client-h1.js') const connectH2 = require('./client-h2.js') @@ -107,7 +108,8 @@ class Client extends DispatcherBase { autoSelectFamilyAttemptTimeout, // h2 maxConcurrentStreams, - allowH2 + allowH2, + pingInterval } = {}) { if (keepAlive !== undefined) { throw new InvalidArgumentError('unsupported keepAlive, use pipelining=0 instead') @@ -199,6 +201,10 @@ class Client extends DispatcherBase { throw new InvalidArgumentError('maxConcurrentStreams must be a positive integer, greater than 0') } + if (pingInterval != null && (typeof pingInterval !== 'number' || !Number.isInteger(pingInterval) || pingInterval < 0)) { + throw new InvalidArgumentError('pingInterval must be a positive integer, greater or equal to 0') + } + super() if (typeof connect !== 'function') { @@ -232,8 +238,10 @@ class Client extends DispatcherBase { this[kMaxRequests] = maxRequestsPerClient this[kClosedResolve] = null this[kMaxResponseSize] = maxResponseSize > -1 ? maxResponseSize : -1 - this[kMaxConcurrentStreams] = maxConcurrentStreams != null ? maxConcurrentStreams : 100 // Max peerConcurrentStreams for a Node h2 server this[kHTTPContext] = null + // h2 + this[kMaxConcurrentStreams] = maxConcurrentStreams != null ? maxConcurrentStreams : 100 // Max peerConcurrentStreams for a Node h2 server + this[kPingInterval] = pingInterval != null ? pingInterval : 60e3 // Default ping interval for h2 - 1 minute // kQueue is built up of 3 sections separated by // the kRunningIdx and kPendingIdx indices. diff --git a/test/http2.js b/test/http2.js index 9b317e15b83..97ea0e778fb 100644 --- a/test/http2.js +++ b/test/http2.js @@ -7,6 +7,7 @@ const { createReadStream, readFileSync } = require('node:fs') const { once } = require('node:events') const { Blob } = require('node:buffer') const { Writable, pipeline, PassThrough, Readable } = require('node:stream') +const { setTimeout: sleep } = require('node:timers/promises') const pem = require('https-pem') @@ -1227,8 +1228,8 @@ test('Should throw informational error on half-closed streams (remote)', async t t = tspl(t, { plan: 4 }) after(async () => { - server.close() await client.close() + server.close() }) await client @@ -1690,8 +1691,8 @@ test('#3803 - sending FormData bodies works', async (t) => { }) t.after(async () => { - server.close() await client.close() + server.close() }) const fd = new FormData() @@ -1818,3 +1819,265 @@ test('Should handle http2 trailers', async t => { await t.completed }) + +test('Should only accept valid ping interval values', async t => { + const planner = tspl(t, { plan: 3 }) + + planner.throws(() => new Client('https://localhost', { + connect: { + rejectUnauthorized: false + }, + allowH2: true, + pingInterval: -1 + })) + + planner.throws(() => new Client('https://localhost', { + connect: { + rejectUnauthorized: false + }, + allowH2: true, + pingInterval: 'foo' + })) + + planner.throws(() => new Client('https://localhost', { + connect: { + rejectUnauthorized: false + }, + allowH2: true, + pingInterval: 1.1 + })) + + await planner.completed +}) + +test('Should send http2 PING frames', async t => { + const server = createSecureServer(pem) + let session = null + let pingCounter = 0 + + server.on('stream', async (stream, headers) => { + stream.respond({ + 'content-type': 'text/plain; charset=utf-8', + 'x-custom-h2': headers['x-my-header'], + ':status': 200 + }, + { + waitForTrailers: true + }) + + stream.on('wantTrailers', () => { + stream.sendTrailers({ + 'x-trailer': 'hello' + }) + }) + + stream.end('hello h2!') + }) + + server.on('session', (s) => { + session = s + session.on('ping', (payload) => { + pingCounter++ + }) + }) + + t = tspl(t, { plan: 2 }) + + server.listen(0, '127.0.0.1') + await once(server, 'listening') + + const client = new Client(`https://${server.address().address}:${server.address().port}`, { + connect: { + rejectUnauthorized: false + }, + allowH2: true, + pingInterval: 100 + }) + + after(async () => { + server.close() + }) + + client.dispatch({ + path: '/', + method: 'PUT', + body: 'hello' + }, { + onConnect () { + + }, + onHeaders () { + return true + }, + onData () { + return true + }, + onComplete (trailers) { + t.strictEqual(trailers['x-trailer'], 'hello') + }, + onError (err) { + t.ifError(err) + } + }) + + await sleep(600) + await client.close() + t.equal(pingCounter, 5, 'Expected 5 PING frames to be sent') + + await t.completed +}) + +test('Should not send http2 PING frames if interval === 0', async t => { + const server = createSecureServer(pem) + let session = null + let pingCounter = 0 + + server.on('stream', async (stream, headers) => { + stream.respond({ + 'content-type': 'text/plain; charset=utf-8', + 'x-custom-h2': headers['x-my-header'], + ':status': 200 + }, + { + waitForTrailers: true + }) + + stream.on('wantTrailers', () => { + stream.sendTrailers({ + 'x-trailer': 'hello' + }) + }) + + stream.end('hello h2!') + }) + + server.on('session', (s) => { + session = s + session.on('ping', (payload) => { + pingCounter++ + }) + }) + + t = tspl(t, { plan: 2 }) + + server.listen(0, '127.0.0.1') + await once(server, 'listening') + + const client = new Client(`https://${server.address().address}:${server.address().port}`, { + connect: { + rejectUnauthorized: false + }, + allowH2: true, + pingInterval: 0 + }) + + after(async () => { + server.close() + }) + + client.dispatch({ + path: '/', + method: 'PUT', + body: 'hello' + }, { + onConnect () { + + }, + onHeaders () { + return true + }, + onData () { + return true + }, + onComplete (trailers) { + t.strictEqual(trailers['x-trailer'], 'hello') + }, + onError (err) { + t.ifError(err) + } + }) + + await sleep(500) + await client.close() + t.equal(pingCounter, 0, 'Expected 0 PING frames to be sent') + + await t.completed +}) + +test('Should not send http2 PING frames after connection is closed', async t => { + const server = createSecureServer(pem) + let session = null + let pingCounter = 0 + + server.on('stream', async (stream, headers) => { + stream.respond({ + 'content-type': 'text/plain; charset=utf-8', + 'x-custom-h2': headers['x-my-header'], + ':status': 200 + }, + { + waitForTrailers: true + }) + + stream.on('wantTrailers', () => { + stream.sendTrailers({ + 'x-trailer': 'hello' + }) + }) + + stream.end('hello h2!') + }) + + server.on('session', (s) => { + session = s + session.on('ping', (payload) => { + pingCounter++ + }) + }) + + t = tspl(t, { plan: 2 }) + + server.listen(0, '127.0.0.1') + await once(server, 'listening') + + const client = new Client(`https://${server.address().address}:${server.address().port}`, { + connect: { + rejectUnauthorized: false + }, + allowH2: true, + pingInterval: 0 + }) + + after(async () => { + session.close() + server.close() + }) + + client.dispatch({ + path: '/', + method: 'PUT', + body: 'hello' + }, { + onConnect () { + + }, + onHeaders () { + return true + }, + onData () { + return true + }, + onComplete (trailers) { + t.strictEqual(trailers['x-trailer'], 'hello') + }, + onError (err) { + t.ifError(err) + } + }) + + await client.close() + await sleep(500) + t.equal(pingCounter, 0, 'Expected 0 PING frames to be sent') + + await t.completed +}) diff --git a/types/client.d.ts b/types/client.d.ts index 088a673eb52..3d31e5be121 100644 --- a/types/client.d.ts +++ b/types/client.d.ts @@ -94,6 +94,11 @@ export declare namespace Client { * @default 100 */ maxConcurrentStreams?: number; + /** + * @description Time interval between PING frames dispatch + * @default 60000 + */ + pingInterval?: number; } export interface SocketInfo { localAddress?: string From 3c606d3a4cc97bb7fcf101441d8e61ffacf94ce5 Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Fri, 4 Jul 2025 10:04:21 +0200 Subject: [PATCH 2/3] docs: extend documentation --- docs/docs/api/Client.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/docs/api/Client.md b/docs/docs/api/Client.md index a915c4a90dc..ae8fdcfde63 100644 --- a/docs/docs/api/Client.md +++ b/docs/docs/api/Client.md @@ -31,7 +31,7 @@ Returns: `Client` * **autoSelectFamilyAttemptTimeout**: `number` - Default: depends on local Node version, on Node 18.13.0 and above is `250`. The amount of time in milliseconds to wait for a connection attempt to finish before trying the next address when using the `autoSelectFamily` option. See [here](https://nodejs.org/api/net.html#socketconnectoptions-connectlistener) for more details. * **allowH2**: `boolean` - Default: `false`. Enables support for H2 if the server has assigned bigger priority to it through ALPN negotiation. * **maxConcurrentStreams**: `number` - Default: `100`. Dictates the maximum number of concurrent streams for a single H2 session. It can be overridden by a SETTINGS remote frame. -* **pingInterval**: `number` - Default: `60e3`. The time interval in milliseconds between PING frames sent to the server. Set to `0` to disable PING frames. This is only applicable for HTTP/2 connections. +* **pingInterval**: `number` - Default: `60e3`. The time interval in milliseconds between PING frames sent to the server. Set to `0` to disable PING frames. This is only applicable for HTTP/2 connections. This will emit a `ping` event on the client with the duration of the ping in milliseconds. > **Notes about HTTP/2** > - It only works under TLS connections. h2c is not supported. From 0cad937cde992ddd92b7e696acf11fb2716e0904 Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Fri, 4 Jul 2025 10:04:36 +0200 Subject: [PATCH 3/3] fix: code review --- lib/dispatcher/client-h2.js | 8 +++++--- test/types/client.test-d.ts | 5 +++++ 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/lib/dispatcher/client-h2.js b/lib/dispatcher/client-h2.js index 23a73533613..c4630da69da 100644 --- a/lib/dispatcher/client-h2.js +++ b/lib/dispatcher/client-h2.js @@ -95,7 +95,7 @@ async function connectH2 (client, socket) { session[kSocket] = socket session[kHTTP2SessionState] = { ping: { - interval: client[kPingInterval] === 0 ? null : setInterval(onHttp2SendPing, client[kPingInterval], session) + interval: client[kPingInterval] === 0 ? null : setInterval(onHttp2SendPing, client[kPingInterval], session).unref() } } @@ -169,10 +169,12 @@ function onHttp2SendPing (session) { function onPing (err, duration) { const client = this[kClient] + const socket = this[kClient] if (err != null) { - // TODO: should we drop the session? - client[kOnError](err) + const error = new InformationalError(`HTTP/2: "PING" errored - type ${err.message}`) + socket[kError] = error + client[kOnError](error) } else { client.emit('ping', duration) } diff --git a/test/types/client.test-d.ts b/test/types/client.test-d.ts index 61168a1a5bb..6a4de6dce49 100644 --- a/test/types/client.test-d.ts +++ b/test/types/client.test-d.ts @@ -105,6 +105,11 @@ expectAssignable( autoSelectFamilyAttemptTimeout: 300e3 }) ) +expectAssignable( + new Client('', { + pingInterval: 60e3 + }) +) expectAssignable( new Client('', { interceptors: {