From f3d1d3352e28a8fe6f535f9459c2278c153b0599 Mon Sep 17 00:00:00 2001 From: Nikolay Karadzhov Date: Tue, 20 May 2025 14:28:15 +0300 Subject: [PATCH] feat(client): expose socketTimeout option (#2965) The maximum duration (in milliseconds) that the socket can remain idle (i.e., with no data sent or received) before being automatically closed. Default reconnectionStrategy will ignore the new SocketTimeoutError, but users are allowed to have custom strategies wich handle those errors in different ways --- docs/client-configuration.md | 8 ++- packages/client/lib/client/socket.spec.ts | 69 +++++++++++++++++++++-- packages/client/lib/client/socket.ts | 24 +++++++- packages/client/lib/errors.ts | 6 ++ 4 files changed, 99 insertions(+), 8 deletions(-) diff --git a/docs/client-configuration.md b/docs/client-configuration.md index 0564794ac4..1c9ba51a11 100644 --- a/docs/client-configuration.md +++ b/docs/client-configuration.md @@ -9,6 +9,7 @@ | socket.family | `0` | IP Stack version (one of `4 \| 6 \| 0`) | | socket.path | | Path to the UNIX Socket | | socket.connectTimeout | `5000` | Connection timeout (in milliseconds) | +| socket.socketTimeout | | The maximum duration (in milliseconds) that the socket can remain idle (i.e., with no data sent or received) before being automatically closed | | socket.noDelay | `true` | Toggle [`Nagle's algorithm`](https://nodejs.org/api/net.html#net_socket_setnodelay_nodelay) | | socket.keepAlive | `true` | Toggle [`keep-alive`](https://nodejs.org/api/net.html#socketsetkeepaliveenable-initialdelay) functionality | | socket.keepAliveInitialDelay | `5000` | If set to a positive number, it sets the initial delay before the first keepalive probe is sent on an idle socket | @@ -40,7 +41,12 @@ By default the strategy uses exponential backoff, but it can be overwritten like ```javascript createClient({ socket: { - reconnectStrategy: retries => { + reconnectStrategy: (retries, cause) => { + // By default, do not reconnect on socket timeout. + if (cause instanceof SocketTimeoutError) { + return false; + } + // Generate a random jitter between 0 – 200 ms: const jitter = Math.floor(Math.random() * 200); // Delay is an exponential back off, (times^2) * 50 ms, with a maximum value of 2000 ms: diff --git a/packages/client/lib/client/socket.spec.ts b/packages/client/lib/client/socket.spec.ts index 20b238a3a3..5117cc4f49 100644 --- a/packages/client/lib/client/socket.spec.ts +++ b/packages/client/lib/client/socket.spec.ts @@ -2,13 +2,12 @@ import { strict as assert } from 'node:assert'; import { spy } from 'sinon'; import { once } from 'node:events'; import RedisSocket, { RedisSocketOptions } from './socket'; +import testUtils, { GLOBAL } from '../test-utils'; +import { setTimeout } from 'timers/promises'; describe('Socket', () => { function createSocket(options: RedisSocketOptions): RedisSocket { - const socket = new RedisSocket( - () => Promise.resolve(), - options - ); + const socket = new RedisSocket(() => Promise.resolve(), options); socket.on('error', () => { // ignore errors @@ -84,4 +83,66 @@ describe('Socket', () => { assert.equal(socket.isOpen, false); }); }); + + describe('socketTimeout', () => { + const timeout = 50; + testUtils.testWithClient( + 'should timeout with positive socketTimeout values', + async client => { + let timedOut = false; + + assert.equal(client.isReady, true, 'client.isReady'); + assert.equal(client.isOpen, true, 'client.isOpen'); + + client.on('error', err => { + assert.equal( + err.message, + `Socket timeout timeout. Expecting data, but didn't receive any in ${timeout}ms.` + ); + + assert.equal(client.isReady, false, 'client.isReady'); + + // This is actually a bug with the onSocketError implementation, + // the client should be closed before the error is emitted + process.nextTick(() => { + assert.equal(client.isOpen, false, 'client.isOpen'); + }); + + timedOut = true; + }); + await setTimeout(timeout * 2); + if (!timedOut) assert.fail('Should have timed out by now'); + }, + { + ...GLOBAL.SERVERS.OPEN, + clientOptions: { + socket: { + socketTimeout: timeout + } + } + } + ); + + testUtils.testWithClient( + 'should not timeout with undefined socketTimeout', + async client => { + + assert.equal(client.isReady, true, 'client.isReady'); + assert.equal(client.isOpen, true, 'client.isOpen'); + + client.on('error', err => { + assert.fail('Should not have timed out or errored in any way'); + }); + await setTimeout(100); + }, + { + ...GLOBAL.SERVERS.OPEN, + clientOptions: { + socket: { + socketTimeout: undefined + } + } + } + ); + }); }); diff --git a/packages/client/lib/client/socket.ts b/packages/client/lib/client/socket.ts index 603416cf9e..58ccbe0b0c 100644 --- a/packages/client/lib/client/socket.ts +++ b/packages/client/lib/client/socket.ts @@ -1,7 +1,7 @@ import { EventEmitter, once } from 'node:events'; import net from 'node:net'; import tls from 'node:tls'; -import { ConnectionTimeoutError, ClientClosedError, SocketClosedUnexpectedlyError, ReconnectStrategyError } from '../errors'; +import { ConnectionTimeoutError, ClientClosedError, SocketClosedUnexpectedlyError, ReconnectStrategyError, SocketTimeoutError } from '../errors'; import { setTimeout } from 'node:timers/promises'; import { RedisArgument } from '../RESP/types'; @@ -23,6 +23,10 @@ type RedisSocketOptionsCommon = { * 3. `(retries: number, cause: Error) => false | number | Error` -> `number` is the same as configuring a `number` directly, `Error` is the same as `false`, but with a custom error. */ reconnectStrategy?: false | number | ReconnectStrategyFunction; + /** + * The timeout (in milliseconds) after which the socket will be closed. `undefined` means no timeout. + */ + socketTimeout?: number; } type RedisTcpOptions = RedisSocketOptionsCommon & NetOptions & Omit< @@ -55,6 +59,7 @@ export default class RedisSocket extends EventEmitter { readonly #connectTimeout; readonly #reconnectStrategy; readonly #socketFactory; + readonly #socketTimeout; #socket?: net.Socket | tls.TLSSocket; @@ -85,6 +90,7 @@ export default class RedisSocket extends EventEmitter { this.#connectTimeout = options?.connectTimeout ?? 5000; this.#reconnectStrategy = this.#createReconnectStrategy(options); this.#socketFactory = this.#createSocketFactory(options); + this.#socketTimeout = options?.socketTimeout; } #createReconnectStrategy(options?: RedisSocketOptions): ReconnectStrategyFunction { @@ -103,7 +109,7 @@ export default class RedisSocket extends EventEmitter { return retryIn; } catch (err) { this.emit('error', err); - return this.defaultReconnectStrategy(retries); + return this.defaultReconnectStrategy(retries, err); } }; } @@ -253,6 +259,13 @@ export default class RedisSocket extends EventEmitter { socket.removeListener('timeout', onTimeout); } + if (this.#socketTimeout) { + socket.once('timeout', () => { + socket.destroy(new SocketTimeoutError(this.#socketTimeout!)); + }); + socket.setTimeout(this.#socketTimeout); + } + socket .once('error', err => this.#onSocketError(err)) .once('close', hadError => { @@ -341,7 +354,12 @@ export default class RedisSocket extends EventEmitter { this.#socket?.unref(); } - defaultReconnectStrategy(retries: number) { + defaultReconnectStrategy(retries: number, cause: unknown) { + // By default, do not reconnect on socket timeout. + if (cause instanceof SocketTimeoutError) { + return false; + } + // Generate a random jitter between 0 – 200 ms: const jitter = Math.floor(Math.random() * 200); // Delay is an exponential back off, (times^2) * 50 ms, with a maximum value of 2000 ms: diff --git a/packages/client/lib/errors.ts b/packages/client/lib/errors.ts index 8af4c5e5be..db37ec1a9b 100644 --- a/packages/client/lib/errors.ts +++ b/packages/client/lib/errors.ts @@ -16,6 +16,12 @@ export class ConnectionTimeoutError extends Error { } } +export class SocketTimeoutError extends Error { + constructor(timeout: number) { + super(`Socket timeout timeout. Expecting data, but didn't receive any in ${timeout}ms.`); + } +} + export class ClientClosedError extends Error { constructor() { super('The client is closed');