From 802ff93d7d72f5bf79971500d9e8678a89385f20 Mon Sep 17 00:00:00 2001 From: Sean Kelly Date: Thu, 2 Feb 2023 22:21:37 -0800 Subject: [PATCH 1/5] Add tcp keepalive interval option --- docs/api/Dispatcher.md | 1 + lib/client.js | 15 +++++++++++++-- lib/core/symbols.js | 1 + types/client.d.ts | 2 ++ 4 files changed, 17 insertions(+), 2 deletions(-) diff --git a/docs/api/Dispatcher.md b/docs/api/Dispatcher.md index 25c980d612b..4a044369704 100644 --- a/docs/api/Dispatcher.md +++ b/docs/api/Dispatcher.md @@ -202,6 +202,7 @@ Returns: `Boolean` - `false` if dispatcher is busy and further dispatch calls wo * **bodyTimeout** `number | null` (optional) - The timeout after which a request will time out, in milliseconds. Monitors time between receiving body data. Use `0` to disable it entirely. Defaults to 30 seconds. * **headersTimeout** `number | null` (optional) - The amount of time the parser will wait to receive the complete HTTP headers while not sending the request. Defaults to 30 seconds. * **throwOnError** `boolean` (optional) - Default: `false` - Whether Undici should throw an error upon receiving a 4xx or 5xx response from the server. +* **tcpKeepAliveInterval** `number | null` (optional) - Default: `60000` - TCP keepalive interval for the socket in milliseconds #### Parameter: `DispatchHandler` diff --git a/lib/client.js b/lib/client.js index 6300c1dd8bb..f0bf627efd2 100644 --- a/lib/client.js +++ b/lib/client.js @@ -63,7 +63,8 @@ const { kDispatch, kInterceptors, kLocalAddress, - kMaxResponseSize + kMaxResponseSize, + kTcpKeepAliveInterval } = require('./core/symbols') const kClosedResolve = Symbol('kClosedResolve') @@ -107,7 +108,8 @@ class Client extends DispatcherBase { connect, maxRequestsPerClient, localAddress, - maxResponseSize + maxResponseSize, + tcpKeepAliveInterval } = {}) { super() @@ -163,6 +165,10 @@ class Client extends DispatcherBase { throw new InvalidArgumentError('bodyTimeout must be a positive integer or zero') } + if (tcpKeepAliveInterval != null && (!Number.isInteger(tcpKeepAliveInterval) || tcpKeepAliveInterval < 0)) { + throw new InvalidArgumentError('tcpKeepAliveInterval must be a positive integer or zero') + } + if (connect != null && typeof connect !== 'function' && typeof connect !== 'object') { throw new InvalidArgumentError('connect must be a function or an object') } @@ -212,6 +218,7 @@ class Client extends DispatcherBase { this[kHostHeader] = `host: ${this[kUrl].hostname}${this[kUrl].port ? `:${this[kUrl].port}` : ''}\r\n` this[kBodyTimeout] = bodyTimeout != null ? bodyTimeout : 30e3 this[kHeadersTimeout] = headersTimeout != null ? headersTimeout : 30e3 + this[kTcpKeepAliveInterval] = tcpKeepAliveInterval != null ? tcpKeepAliveInterval : 60e3 this[kStrictContentLength] = strictContentLength == null ? true : strictContentLength this[kMaxRedirections] = maxRedirections this[kMaxRequests] = maxRequestsPerClient @@ -1349,6 +1356,10 @@ function write (client, request) { const socket = client[kSocket] + if (client[kTcpKeepAliveInterval]) { + socket.setKeepAlive(true, client[kTcpKeepAliveInterval]) + } + try { request.onConnect((err) => { if (request.aborted || request.completed) { diff --git a/lib/core/symbols.js b/lib/core/symbols.js index 6d6b62919e6..f7eebb801bb 100644 --- a/lib/core/symbols.js +++ b/lib/core/symbols.js @@ -16,6 +16,7 @@ module.exports = { kKeepAlive: Symbol('keep alive'), kHeadersTimeout: Symbol('headers timeout'), kBodyTimeout: Symbol('body timeout'), + kTcpKeepAliveInterval: Symbol('tcp keep alive interval'), kServerName: Symbol('server name'), kLocalAddress: Symbol('local address'), kHost: Symbol('host'), diff --git a/types/client.d.ts b/types/client.d.ts index 3a954ac83a1..bd54ce528e9 100644 --- a/types/client.d.ts +++ b/types/client.d.ts @@ -43,6 +43,8 @@ declare namespace Client { maxRequestsPerClient?: number; /** Max response body size in bytes, -1 is disabled */ maxResponseSize?: number | null; + /** TCP keepalive interval for the socket in milliseconds. Default: `60e3` milliseconds (60s) */ + tcpKeepAliveInterval?: number | null; interceptors?: {Client: readonly DispatchInterceptor[] | undefined} } From 2956c167fddf23e3de595ea9391044911838d379 Mon Sep 17 00:00:00 2001 From: Sean Kelly Date: Fri, 3 Feb 2023 09:54:56 -0800 Subject: [PATCH 2/5] Move config option, this can be accomplished with just connect() options already? --- docs/api/Client.md | 1 + lib/client.js | 9 ++------- lib/core/connect.js | 8 +++++++- lib/core/symbols.js | 1 - 4 files changed, 10 insertions(+), 9 deletions(-) diff --git a/docs/api/Client.md b/docs/api/Client.md index 2b5f9e526bb..4408f7727ea 100644 --- a/docs/api/Client.md +++ b/docs/api/Client.md @@ -38,6 +38,7 @@ Furthermore, the following options can be passed: * **maxCachedSessions** `number | null` (optional) - Default: `100` - Maximum number of TLS cached sessions. Use 0 to disable TLS session caching. Default: 100. * **timeout** `number | null` (optional) - Default `10e3` * **servername** `string | null` (optional) +* **tcpKeepAliveInterval** `number | null` (optional) - Default: `60000` - TCP keepalive interval for the socket in milliseconds ### Example - Basic Client instantiation diff --git a/lib/client.js b/lib/client.js index f0bf627efd2..655b6169ed3 100644 --- a/lib/client.js +++ b/lib/client.js @@ -63,8 +63,7 @@ const { kDispatch, kInterceptors, kLocalAddress, - kMaxResponseSize, - kTcpKeepAliveInterval + kMaxResponseSize } = require('./core/symbols') const kClosedResolve = Symbol('kClosedResolve') @@ -195,6 +194,7 @@ class Client extends DispatcherBase { maxCachedSessions, socketPath, timeout: connectTimeout, + tcpKeepAliveInterval, ...connect }) } @@ -218,7 +218,6 @@ class Client extends DispatcherBase { this[kHostHeader] = `host: ${this[kUrl].hostname}${this[kUrl].port ? `:${this[kUrl].port}` : ''}\r\n` this[kBodyTimeout] = bodyTimeout != null ? bodyTimeout : 30e3 this[kHeadersTimeout] = headersTimeout != null ? headersTimeout : 30e3 - this[kTcpKeepAliveInterval] = tcpKeepAliveInterval != null ? tcpKeepAliveInterval : 60e3 this[kStrictContentLength] = strictContentLength == null ? true : strictContentLength this[kMaxRedirections] = maxRedirections this[kMaxRequests] = maxRequestsPerClient @@ -1356,10 +1355,6 @@ function write (client, request) { const socket = client[kSocket] - if (client[kTcpKeepAliveInterval]) { - socket.setKeepAlive(true, client[kTcpKeepAliveInterval]) - } - try { request.onConnect((err) => { if (request.aborted || request.completed) { diff --git a/lib/core/connect.js b/lib/core/connect.js index 89561f16fbe..7a01a09bdf6 100644 --- a/lib/core/connect.js +++ b/lib/core/connect.js @@ -71,7 +71,7 @@ if (global.FinalizationRegistry) { } } -function buildConnector ({ maxCachedSessions, socketPath, timeout, ...opts }) { +function buildConnector ({ maxCachedSessions, socketPath, timeout, tcpKeepAliveInterval, ...opts }) { if (maxCachedSessions != null && (!Number.isInteger(maxCachedSessions) || maxCachedSessions < 0)) { throw new InvalidArgumentError('maxCachedSessions must be a positive integer or zero') } @@ -80,6 +80,8 @@ function buildConnector ({ maxCachedSessions, socketPath, timeout, ...opts }) { const sessionCache = new SessionCache(maxCachedSessions == null ? 100 : maxCachedSessions) timeout = timeout == null ? 10e3 : timeout + tcpKeepAliveInterval = tcpKeepAliveInterval == null ? 60e3 : tcpKeepAliveInterval + return function connect ({ hostname, host, protocol, port, servername, localAddress, httpSocket }, callback) { let socket if (protocol === 'https:') { @@ -95,6 +97,8 @@ function buildConnector ({ maxCachedSessions, socketPath, timeout, ...opts }) { socket = tls.connect({ highWaterMark: 16384, // TLS in node can't have bigger HWM anyway... + keepAlive: tcpKeepAliveInterval !== 0, + keepAliveInitialDelay: tcpKeepAliveInterval, ...options, servername, session, @@ -113,6 +117,8 @@ function buildConnector ({ maxCachedSessions, socketPath, timeout, ...opts }) { assert(!httpSocket, 'httpSocket can only be sent on TLS update') socket = net.connect({ highWaterMark: 64 * 1024, // Same as nodejs fs streams. + keepAlive: tcpKeepAliveInterval !== 0, + keepAliveInitialDelay: tcpKeepAliveInterval, ...options, localAddress, port: port || 80, diff --git a/lib/core/symbols.js b/lib/core/symbols.js index f7eebb801bb..6d6b62919e6 100644 --- a/lib/core/symbols.js +++ b/lib/core/symbols.js @@ -16,7 +16,6 @@ module.exports = { kKeepAlive: Symbol('keep alive'), kHeadersTimeout: Symbol('headers timeout'), kBodyTimeout: Symbol('body timeout'), - kTcpKeepAliveInterval: Symbol('tcp keep alive interval'), kServerName: Symbol('server name'), kLocalAddress: Symbol('local address'), kHost: Symbol('host'), From a44b8abed5ad46c6eb1be4e0e094a4aca83ad0ed Mon Sep 17 00:00:00 2001 From: Sean Kelly Date: Fri, 3 Feb 2023 09:57:27 -0800 Subject: [PATCH 3/5] Move config option, this can be accomplished with just connect() options already? --- docs/api/Dispatcher.md | 1 - types/client.d.ts | 2 -- types/connector.d.ts | 2 ++ 3 files changed, 2 insertions(+), 3 deletions(-) diff --git a/docs/api/Dispatcher.md b/docs/api/Dispatcher.md index 4a044369704..25c980d612b 100644 --- a/docs/api/Dispatcher.md +++ b/docs/api/Dispatcher.md @@ -202,7 +202,6 @@ Returns: `Boolean` - `false` if dispatcher is busy and further dispatch calls wo * **bodyTimeout** `number | null` (optional) - The timeout after which a request will time out, in milliseconds. Monitors time between receiving body data. Use `0` to disable it entirely. Defaults to 30 seconds. * **headersTimeout** `number | null` (optional) - The amount of time the parser will wait to receive the complete HTTP headers while not sending the request. Defaults to 30 seconds. * **throwOnError** `boolean` (optional) - Default: `false` - Whether Undici should throw an error upon receiving a 4xx or 5xx response from the server. -* **tcpKeepAliveInterval** `number | null` (optional) - Default: `60000` - TCP keepalive interval for the socket in milliseconds #### Parameter: `DispatchHandler` diff --git a/types/client.d.ts b/types/client.d.ts index bd54ce528e9..3a954ac83a1 100644 --- a/types/client.d.ts +++ b/types/client.d.ts @@ -43,8 +43,6 @@ declare namespace Client { maxRequestsPerClient?: number; /** Max response body size in bytes, -1 is disabled */ maxResponseSize?: number | null; - /** TCP keepalive interval for the socket in milliseconds. Default: `60e3` milliseconds (60s) */ - tcpKeepAliveInterval?: number | null; interceptors?: {Client: readonly DispatchInterceptor[] | undefined} } diff --git a/types/connector.d.ts b/types/connector.d.ts index 2b28771af2a..43fe9d37855 100644 --- a/types/connector.d.ts +++ b/types/connector.d.ts @@ -10,6 +10,8 @@ declare namespace buildConnector { socketPath?: string | null; timeout?: number | null; port?: number; + /** TCP keepalive interval for the socket in milliseconds. Default: `60e3` milliseconds (60s) */ + tcpKeepAliveInterval?: number | null; } export interface Options { From 1976091a3b579ce59f6d30112b3c3f7c5ffd7ccd Mon Sep 17 00:00:00 2001 From: Sean Kelly Date: Fri, 3 Feb 2023 10:54:00 -0800 Subject: [PATCH 4/5] Simplify changes --- docs/api/Client.md | 3 ++- lib/client.js | 8 +------- lib/core/connect.js | 14 +++++++------- types/connector.d.ts | 2 -- 4 files changed, 10 insertions(+), 17 deletions(-) diff --git a/docs/api/Client.md b/docs/api/Client.md index 4408f7727ea..1aca7d09d31 100644 --- a/docs/api/Client.md +++ b/docs/api/Client.md @@ -38,7 +38,8 @@ Furthermore, the following options can be passed: * **maxCachedSessions** `number | null` (optional) - Default: `100` - Maximum number of TLS cached sessions. Use 0 to disable TLS session caching. Default: 100. * **timeout** `number | null` (optional) - Default `10e3` * **servername** `string | null` (optional) -* **tcpKeepAliveInterval** `number | null` (optional) - Default: `60000` - TCP keepalive interval for the socket in milliseconds +* **keepAlive** `boolean | null` (optional) - Default: `true` - TCP keep-alive enabled +* **keepAliveInitialDelay** `number | null` (optional) - Default: `60000` - TCP keep-alive interval for the socket in milliseconds ### Example - Basic Client instantiation diff --git a/lib/client.js b/lib/client.js index 655b6169ed3..6300c1dd8bb 100644 --- a/lib/client.js +++ b/lib/client.js @@ -107,8 +107,7 @@ class Client extends DispatcherBase { connect, maxRequestsPerClient, localAddress, - maxResponseSize, - tcpKeepAliveInterval + maxResponseSize } = {}) { super() @@ -164,10 +163,6 @@ class Client extends DispatcherBase { throw new InvalidArgumentError('bodyTimeout must be a positive integer or zero') } - if (tcpKeepAliveInterval != null && (!Number.isInteger(tcpKeepAliveInterval) || tcpKeepAliveInterval < 0)) { - throw new InvalidArgumentError('tcpKeepAliveInterval must be a positive integer or zero') - } - if (connect != null && typeof connect !== 'function' && typeof connect !== 'object') { throw new InvalidArgumentError('connect must be a function or an object') } @@ -194,7 +189,6 @@ class Client extends DispatcherBase { maxCachedSessions, socketPath, timeout: connectTimeout, - tcpKeepAliveInterval, ...connect }) } diff --git a/lib/core/connect.js b/lib/core/connect.js index 7a01a09bdf6..709295af79d 100644 --- a/lib/core/connect.js +++ b/lib/core/connect.js @@ -71,7 +71,7 @@ if (global.FinalizationRegistry) { } } -function buildConnector ({ maxCachedSessions, socketPath, timeout, tcpKeepAliveInterval, ...opts }) { +function buildConnector ({ maxCachedSessions, socketPath, timeout, ...opts }) { if (maxCachedSessions != null && (!Number.isInteger(maxCachedSessions) || maxCachedSessions < 0)) { throw new InvalidArgumentError('maxCachedSessions must be a positive integer or zero') } @@ -80,8 +80,6 @@ function buildConnector ({ maxCachedSessions, socketPath, timeout, tcpKeepAliveI const sessionCache = new SessionCache(maxCachedSessions == null ? 100 : maxCachedSessions) timeout = timeout == null ? 10e3 : timeout - tcpKeepAliveInterval = tcpKeepAliveInterval == null ? 60e3 : tcpKeepAliveInterval - return function connect ({ hostname, host, protocol, port, servername, localAddress, httpSocket }, callback) { let socket if (protocol === 'https:') { @@ -97,8 +95,6 @@ function buildConnector ({ maxCachedSessions, socketPath, timeout, tcpKeepAliveI socket = tls.connect({ highWaterMark: 16384, // TLS in node can't have bigger HWM anyway... - keepAlive: tcpKeepAliveInterval !== 0, - keepAliveInitialDelay: tcpKeepAliveInterval, ...options, servername, session, @@ -117,8 +113,6 @@ function buildConnector ({ maxCachedSessions, socketPath, timeout, tcpKeepAliveI assert(!httpSocket, 'httpSocket can only be sent on TLS update') socket = net.connect({ highWaterMark: 64 * 1024, // Same as nodejs fs streams. - keepAlive: tcpKeepAliveInterval !== 0, - keepAliveInitialDelay: tcpKeepAliveInterval, ...options, localAddress, port: port || 80, @@ -126,6 +120,12 @@ function buildConnector ({ maxCachedSessions, socketPath, timeout, tcpKeepAliveI }) } + // Set TCP keep alive options on the socket here instead of in connect() for the case of assigning the socket + if (options.keepAlive ?? true) { + const keepAliveInitialDelay = options.keepAliveInitialDelay === undefined ? 60e3 : options.keepAliveInitialDelay + socket.setKeepAlive(true, keepAliveInitialDelay) + } + const cancelTimeout = setupTimeout(() => onConnectTimeout(socket), timeout) socket diff --git a/types/connector.d.ts b/types/connector.d.ts index 43fe9d37855..2b28771af2a 100644 --- a/types/connector.d.ts +++ b/types/connector.d.ts @@ -10,8 +10,6 @@ declare namespace buildConnector { socketPath?: string | null; timeout?: number | null; port?: number; - /** TCP keepalive interval for the socket in milliseconds. Default: `60e3` milliseconds (60s) */ - tcpKeepAliveInterval?: number | null; } export interface Options { From 67ac647dc52d880249d62211772454d52700c585 Mon Sep 17 00:00:00 2001 From: Sean Kelly Date: Fri, 3 Feb 2023 11:19:53 -0800 Subject: [PATCH 5/5] Node 12 compatibility, oops --- lib/core/connect.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/core/connect.js b/lib/core/connect.js index 709295af79d..f3b5cc33edd 100644 --- a/lib/core/connect.js +++ b/lib/core/connect.js @@ -121,7 +121,7 @@ function buildConnector ({ maxCachedSessions, socketPath, timeout, ...opts }) { } // Set TCP keep alive options on the socket here instead of in connect() for the case of assigning the socket - if (options.keepAlive ?? true) { + if (options.keepAlive == null || options.keepAlive) { const keepAliveInitialDelay = options.keepAliveInitialDelay === undefined ? 60e3 : options.keepAliveInitialDelay socket.setKeepAlive(true, keepAliveInitialDelay) }