diff --git a/README.md b/README.md index 5a96c2061f6..d7c4658842e 100644 --- a/README.md +++ b/README.md @@ -388,6 +388,7 @@ The Node Redis client class is an Nodejs EventEmitter and it emits an event each | `end` | Connection has been closed (via `.close()` or `.destroy()`) | _No arguments_ | | `error` | An error has occurred—usually a network issue such as "Socket closed unexpectedly" | `(error: Error)` | | `reconnecting` | Client is trying to reconnect to the server | _No arguments_ | +| `terminated` | Reconnection has stopped because `reconnectStrategy` returned `false` or an `Error`; emitted before the companion `error` | `(cause: Error)` | | `sharded-channel-moved` | See [here](https://github.com/redis/node-redis/blob/master/docs/pub-sub.md#sharded-channel-moved-event) | See [here](https://github.com/redis/node-redis/blob/master/docs/pub-sub.md#sharded-channel-moved-event) | | `invalidate` | Client Tracking is on with `emitInvalidate` and a key is invalidated | `(key: RedisItem \| null)` | diff --git a/docs/client-configuration.md b/docs/client-configuration.md index 46d351bc2eb..d2169853b92 100644 --- a/docs/client-configuration.md +++ b/docs/client-configuration.md @@ -62,6 +62,15 @@ createClient({ }); ``` +An `'error'` event fires on every disconnect, including ones the client is about to retry, so it can't tell you whether reconnection is still in progress. Once `reconnectStrategy` gives up (returns `false` or an `Error`), the client emits a `'terminated'` event before the companion `'error'` event. This ordering lets `await events.once(client, 'terminated')` resolve with the cause before the companion error is emitted, and distinguishes a permanent termination from a transient retry. Call `destroy()` before replacing the client so its resources are released: + +```javascript +client.on('terminated', cause => { + console.error('client will not reconnect:', cause); + queueMicrotask(() => client.destroy()); +}); +``` + ## TLS To enable TLS, set `socket.tls` to `true`. Below are some basic examples. diff --git a/packages/client/lib/client/index.spec.ts b/packages/client/lib/client/index.spec.ts index 2621c7de7ca..a15dafe2377 100644 --- a/packages/client/lib/client/index.spec.ts +++ b/packages/client/lib/client/index.spec.ts @@ -1551,6 +1551,26 @@ describe('Client', () => { describe("socket errors during handshake", () => { + it("should re-emit terminated from the socket", async () => { + const client = createClient({ + socket: { + host: "error", + connectTimeout: 1, + reconnectStrategy: false + } + }); + client.on("error", () => {}); + + const terminated = new Promise(resolve => { + client.once("terminated", resolve); + }); + await assert.rejects(client.connect()); + + const cause = await terminated; + assert.ok(cause instanceof Error); + client.destroy(); + }); + it("should successfully connect when server accepts connection immediately", async () => { const { log, client, teardown } = await setup({}, 0); await client.connect(); diff --git a/packages/client/lib/client/index.ts b/packages/client/lib/client/index.ts index d9ab2eab422..dd3dfa12157 100644 --- a/packages/client/lib/client/index.ts +++ b/packages/client/lib/client/index.ts @@ -1053,6 +1053,7 @@ export default class RedisClient< this.#maybeScheduleWrite(); }) .on('reconnecting', () => this.emit('reconnecting')) + .on('terminated', cause => this.emit('terminated', cause)) .on('drain', () => this.#maybeScheduleWrite()) .on('end', () => this.emit('end')); } diff --git a/packages/client/lib/client/socket.spec.ts b/packages/client/lib/client/socket.spec.ts index 260bc9af97c..b8df7b2757e 100644 --- a/packages/client/lib/client/socket.spec.ts +++ b/packages/client/lib/client/socket.spec.ts @@ -5,6 +5,7 @@ import net from 'node:net'; import RedisSocket, { RedisSocketOptions } from './socket'; import testUtils, { GLOBAL } from '../test-utils'; import { setTimeout } from 'timers/promises'; +import { ReconnectStrategyError } from '../errors'; describe('Socket', () => { const CLIENT_ID = 'test-client-id'; @@ -87,6 +88,182 @@ describe('Socket', () => { }); }); + describe('terminated event (#2948)', () => { + it('should emit `terminated` when reconnectStrategy gives up on the initial connection', async () => { + const socket = createSocket({ + host: 'error', + connectTimeout: 1, + reconnectStrategy: false + }); + + const events: string[] = []; + let terminatedCause: Error | undefined; + socket.on('terminated', cause => { + events.push('terminated'); + terminatedCause = cause; + }); + socket.on('error', () => events.push('error')); + + await assert.rejects(socket.connect()); + + assert.ok(terminatedCause instanceof Error); + assert.deepEqual(events.slice(-2), ['terminated', 'error']); + assert.equal(socket.isOpen, false); + }); + + it('should emit `terminated` with the wrapped error when a custom reconnectStrategy gives up', async () => { + const reconnectStrategy = spy((retries: number) => { + if (retries === 1) return new Error('done'); + return 0; + }); + + const socket = createSocket({ + host: 'error', + connectTimeout: 1, + reconnectStrategy + }); + + const events: string[] = []; + let terminatedCause: Error | undefined; + socket.on('terminated', cause => { + events.push('terminated'); + terminatedCause = cause; + }); + socket.on('error', () => events.push('error')); + + await assert.rejects(socket.connect()); + + assert.ok(terminatedCause instanceof ReconnectStrategyError, 'terminated cause should be a ReconnectStrategyError'); + assert.deepEqual(events.slice(-2), ['terminated', 'error']); + }); + + it('should emit `terminated` — not just `error` — when the connection is lost after being ready', async () => { + // This is the scenario from #2948: `error` fires on *every* disconnect, + // including ones the client is about to retry, so a listener can't tell + // "still retrying" apart from "reconnectStrategy gave up, this client + // is dead". Before this fix, losing an already-ready connection only + // ever emitted `error`, indistinguishable from a transient one. + const connections: net.Socket[] = []; + const server = net.createServer(conn => { + conn.on('error', () => { /* ignore */ }); + connections.push(conn); + }); + await new Promise(resolve => server.listen(0, '127.0.0.1', resolve)); + const { port } = server.address() as net.AddressInfo; + const firstConnection = once(server, 'connection') as Promise<[net.Socket]>; + + try { + const socket = createSocket({ + host: '127.0.0.1', + port, + // Give up as soon as the live connection dies, instead of retrying. + reconnectStrategy: false + }); + + await socket.connect(); + assert.equal(socket.isReady, true, 'socket.isReady'); + + const events: string[] = []; + let terminatedCause: Error | undefined; + socket.on('terminated', cause => { + events.push('terminated'); + terminatedCause = cause; + }); + socket.on('error', () => events.push('error')); + + const [conn] = await firstConnection; + conn.destroy(); + const [errCause] = await once(socket, 'error') as [Error]; + + assert.deepEqual(events, ['terminated', 'error']); + assert.equal(terminatedCause, errCause, 'terminated should carry the same cause as error'); + assert.equal(socket.isOpen, false, 'socket.isOpen'); + } finally { + for (const conn of connections) conn.destroy(); + await new Promise(resolve => server.close(() => resolve())); + } + }); + + it('should not emit `terminated` when the reconnectStrategy schedules a retry', async () => { + const socket = createSocket({ + host: 'error', + connectTimeout: 1, + reconnectStrategy: 0 + }); + + let terminatedCount = 0; + socket.on('terminated', () => terminatedCount++); + + socket.connect(); + await once(socket, 'error'); + assert.equal(socket.isOpen, true); + assert.equal(terminatedCount, 0, 'terminated must not fire while still retrying'); + + socket.destroy(); + }); + + it('should not reconnect when an error listener destroys the socket', async () => { + const connections: net.Socket[] = []; + const server = net.createServer(conn => { + conn.on('error', () => { /* ignore */ }); + connections.push(conn); + }); + await new Promise(resolve => server.listen(0, '127.0.0.1', resolve)); + const { port } = server.address() as net.AddressInfo; + const firstConnection = once(server, 'connection') as Promise<[net.Socket]>; + + try { + const socket = createSocket({ + host: '127.0.0.1', + port, + reconnectStrategy: 0 + }); + socket.on('error', () => socket.destroy()); + + await socket.connect(); + const [conn] = await firstConnection; + conn.destroy(); + await once(socket, 'end'); + await setTimeout(10); + + assert.equal(connections.length, 1, 'destroyed socket must not reconnect'); + assert.equal(socket.isOpen, false, 'socket.isOpen'); + } finally { + for (const conn of connections) conn.destroy(); + await new Promise(resolve => server.close(() => resolve())); + } + }); + + it('should emit `terminated` before `error` when the handshake fails', async () => { + const connections: net.Socket[] = []; + const server = net.createServer(conn => { + conn.on('error', () => { /* ignore */ }); + connections.push(conn); + setImmediate(() => conn.destroy()); + }); + await new Promise(resolve => server.listen(0, '127.0.0.1', resolve)); + const { port } = server.address() as net.AddressInfo; + + try { + const socket = new RedisSocket(() => new Promise(() => { /* wait for socket failure */ }), CLIENT_ID, { + host: '127.0.0.1', + port, + reconnectStrategy: false + }); + const events: string[] = []; + socket.on('terminated', () => events.push('terminated')); + socket.on('error', () => events.push('error')); + + await assert.rejects(socket.connect()); + + assert.deepEqual(events, ['terminated', 'error']); + } finally { + for (const conn of connections) conn.destroy(); + await new Promise(resolve => server.close(() => resolve())); + } + }); + }); + describe('initiator interruption (#3346)', () => { it('should keep retrying when the socket dies while the initiator is suspended', async () => { const connections: net.Socket[] = []; diff --git a/packages/client/lib/client/socket.ts b/packages/client/lib/client/socket.ts index 3c30a2a638e..0bf791c4d01 100644 --- a/packages/client/lib/client/socket.ts +++ b/packages/client/lib/client/socket.ts @@ -13,6 +13,10 @@ type NetOptions = { }; type ReconnectStrategyFunction = (retries: number, cause: Error) => false | Error | number; +type ReconnectStrategyResult = { + retryIn: false | Error | number; + error?: Error; +}; type RedisSocketOptionsCommon = { /** @@ -111,34 +115,30 @@ export default class RedisSocket extends EventEmitter { this.#clientId = clientId; } - #createReconnectStrategy(options?: RedisSocketOptions): ReconnectStrategyFunction { + #createReconnectStrategy(options?: RedisSocketOptions): (retries: number, cause: Error) => ReconnectStrategyResult { const strategy = options?.reconnectStrategy; if (strategy === false || typeof strategy === 'number') { - return () => strategy; + return () => ({ retryIn: strategy }); } if (strategy) { return (retries, cause) => { try { -const retryIn = strategy(retries, cause); + const retryIn = strategy(retries, cause); if (retryIn !== false && !(retryIn instanceof Error) && typeof retryIn !== 'number') { throw new TypeError(`Reconnect strategy should return \`false | Error | number\`, got ${retryIn} instead`); } - return retryIn; + return { retryIn }; } catch (err) { - publish(CHANNELS.ERROR, () => ({ - error: err as Error, - origin: 'client', - internal: false, - clientId: this.#clientId - })); - this.emit('error', err); - return this.defaultReconnectStrategy(retries, err); + return { + retryIn: this.defaultReconnectStrategy(retries, err), + error: err as Error + }; } }; } - return this.defaultReconnectStrategy; + return (retries, cause) => ({ retryIn: this.defaultReconnectStrategy(retries, cause) }); } #createSocketFactory(options?: RedisSocketOptions) { @@ -205,31 +205,39 @@ const retryIn = strategy(retries, cause); }; } + /** + * The single choke point where `reconnectStrategy` giving up (`false` or an + * `Error`) is handled: closes the socket for good and emits `'terminated'` + * so a caller reacting only to `'error'` — which also fires on every + * *retried* disconnect — can tell "still retrying" apart from "reconnection + * has permanently stopped, the client is unusable from here on". + */ #shouldReconnect(retries: number, cause: Error) { - const retryIn = this.#reconnectStrategy(retries, cause); + const { retryIn, error: strategyError } = this.#reconnectStrategy(retries, cause); if (retryIn === false) { this.#isOpen = false; - publish(CHANNELS.ERROR, () => ({ - error: cause, - origin: 'client', - internal: false, - clientId: this.#clientId - })); - this.emit('error', cause); - return cause; + this.emit('terminated', cause); + this.#emitError(cause); + return { retryIn: cause, strategyError }; } else if (retryIn instanceof Error) { this.#isOpen = false; - publish(CHANNELS.ERROR, () => ({ - error: cause, - origin: 'client', - internal: false, - clientId: this.#clientId - })); - this.emit('error', cause); - return new ReconnectStrategyError(retryIn, cause); + const terminatedBy = new ReconnectStrategyError(retryIn, cause); + this.emit('terminated', terminatedBy); + this.#emitError(cause); + return { retryIn: terminatedBy, strategyError }; } - return retryIn; + return { retryIn, strategyError }; + } + + #emitError(err: Error) { + publish(CHANNELS.ERROR, () => ({ + error: err, + origin: 'client', + internal: false, + clientId: this.#clientId + })); + this.emit('error', err); } async connect(): Promise { @@ -254,9 +262,14 @@ const retryIn = strategy(retries, cause); // Check if socket was closed/destroyed during initiator execution if (!this.#socket || this.#socket.destroyed || !this.#socket.readable || !this.#socket.writable) { - const retryIn = this.#shouldReconnect(retries++, new SocketClosedUnexpectedlyError()); + const error = new SocketClosedUnexpectedlyError(); + const { retryIn, strategyError } = this.#shouldReconnect(retries++, error); if (typeof retryIn !== 'number') { throw retryIn; } + this.#emitError(error); + if (strategyError) this.#emitError(strategyError); + if (!this.#isOpen) throw new ClientClosedError(); await setTimeout(retryIn); + if (!this.#isOpen) throw new ClientClosedError(); this.emit('reconnecting'); continue; } @@ -282,19 +295,16 @@ const retryIn = strategy(retries, cause); // reconnecting or scheduling a retry — the shutdown is intentional. if (!this.#isOpen) throw err; - const retryIn = this.#shouldReconnect(retries++, err as Error); + const { retryIn, strategyError } = this.#shouldReconnect(retries++, err as Error); if (typeof retryIn !== 'number') { throw retryIn; } - publish(CHANNELS.ERROR, () => ({ - error: err as Error, - origin: 'client', - internal: false, - clientId: this.#clientId - })); - this.emit('error', err); + this.#emitError(err as Error); + if (strategyError) this.#emitError(strategyError); + if (!this.#isOpen) throw err; await setTimeout(retryIn); + if (!this.#isOpen) throw err; this.emit('reconnecting'); } } while (this.#isOpen && !this.#isReady); @@ -397,14 +407,6 @@ const retryIn = strategy(retries, cause); this.#isReady = false; const socket = this.#socket; this.#socket = undefined; - publish(CHANNELS.ERROR, () => ({ - error: err, - origin: 'client', - internal: false, - clientId: this.#clientId - })); - this.emit('error', err); - socket?.removeAllListeners('data'); socket?.destroy(); @@ -412,7 +414,24 @@ const retryIn = strategy(retries, cause); publish(CHANNELS.CONNECTION_CLOSED, () => ({ clientId: this.#clientId, reason: 'error', wasConnected: true })); } - if (!wasReady || !this.#isOpen || typeof this.#shouldReconnect(0, err) !== 'number') return; + if (!wasReady) { + if (!this.#isOpen) { + this.#emitError(err); + } + return; + } + + if (!this.#isOpen) { + this.#emitError(err); + return; + } + + const { retryIn, strategyError } = this.#shouldReconnect(0, err); + if (typeof retryIn !== 'number') return; + + this.#emitError(err); + if (strategyError) this.#emitError(strategyError); + if (!this.#isOpen) return; this.emit('reconnecting'); this.#connect().catch(() => { diff --git a/packages/redis/README.md b/packages/redis/README.md index 2242229b50b..8e7bfe5eb9a 100644 --- a/packages/redis/README.md +++ b/packages/redis/README.md @@ -302,6 +302,7 @@ The Node Redis client class is an Nodejs EventEmitter and it emits an event each | `end` | Connection has been closed (via `.disconnect()`) | _No arguments_ | | `error` | An error has occurred—usually a network issue such as "Socket closed unexpectedly" | `(error: Error)` | | `reconnecting` | Client is trying to reconnect to the server | _No arguments_ | +| `terminated` | Reconnection has stopped because `reconnectStrategy` returned `false` or an `Error`; emitted before the companion `error` | `(cause: Error)` | | `sharded-channel-moved` | See [here](https://github.com/redis/node-redis/blob/master/docs/pub-sub.md#sharded-channel-moved-event) | See [here](https://github.com/redis/node-redis/blob/master/docs/pub-sub.md#sharded-channel-moved-event) | > :warning: You **MUST** listen to `error` events. If a client doesn't have at least one `error` listener registered and