From 0f28dad2a299df28db0b83785cbfa72c1130ea34 Mon Sep 17 00:00:00 2001 From: Mik13 Date: Fri, 24 Feb 2023 23:33:16 +0100 Subject: [PATCH] Execute empty MULTI (#2423) * Fix multi.exec with empty queue and previous watch When calling exec on a multi instance which you did not use, no command is sent currently. This is a problem for watched keys, because no EXEC means no unwatch, which might cause hard-to-debug problems. Proposed Fix: Sending UNWATCH * execute empty multi command (instead of skipping) * Update index.ts * Update index.ts * Update multi-command.ts * Update multi-command.ts * Update multi-command.ts * Update multi-command.ts * short circuit empty pipelines * Update index.ts --------- Co-authored-by: Leibale --- packages/client/lib/client/index.spec.ts | 25 +++++++++++++------- packages/client/lib/client/index.ts | 21 +++++++++++----- packages/client/lib/client/multi-command.ts | 7 +++--- packages/client/lib/cluster/multi-command.ts | 5 +--- packages/client/lib/multi-command.spec.ts | 21 ++++++++-------- packages/client/lib/multi-command.ts | 12 ---------- 6 files changed, 46 insertions(+), 45 deletions(-) diff --git a/packages/client/lib/client/index.spec.ts b/packages/client/lib/client/index.spec.ts index 25c966c271..7396a2e9c3 100644 --- a/packages/client/lib/client/index.spec.ts +++ b/packages/client/lib/client/index.spec.ts @@ -518,14 +518,23 @@ describe('Client', () => { ); }, GLOBAL.SERVERS.OPEN); - testUtils.testWithClient('execAsPipeline', async client => { - assert.deepEqual( - await client.multi() - .ping() - .exec(true), - ['PONG'] - ); - }, GLOBAL.SERVERS.OPEN); + describe('execAsPipeline', () => { + testUtils.testWithClient('exec(true)', async client => { + assert.deepEqual( + await client.multi() + .ping() + .exec(true), + ['PONG'] + ); + }, GLOBAL.SERVERS.OPEN); + + testUtils.testWithClient('empty execAsPipeline', async client => { + assert.deepEqual( + await client.multi().execAsPipeline(), + [] + ); + }, GLOBAL.SERVERS.OPEN); + }); testUtils.testWithClient('should remember selected db', async client => { await client.multi() diff --git a/packages/client/lib/client/index.ts b/packages/client/lib/client/index.ts index b4bf49fc7b..5dd386647e 100644 --- a/packages/client/lib/client/index.ts +++ b/packages/client/lib/client/index.ts @@ -460,7 +460,7 @@ export default class RedisClient< ); } else if (!this.#socket.isReady && this.#options?.disableOfflineQueue) { return Promise.reject(new ClientOfflineError()); - } + } const promise = this.#queue.addCommand(args, options); this.#tick(); @@ -725,11 +725,14 @@ export default class RedisClient< return Promise.reject(new ClientClosedError()); } - const promise = Promise.all( - commands.map(({ args }) => { - return this.#queue.addCommand(args, { chainId }); - }) - ); + const promise = chainId ? + // if `chainId` has a value, it's a `MULTI` (and not "pipeline") - need to add the `MULTI` and `EXEC` commands + Promise.all([ + this.#queue.addCommand(['MULTI'], { chainId }), + this.#addMultiCommands(commands, chainId), + this.#queue.addCommand(['EXEC'], { chainId }) + ]) : + this.#addMultiCommands(commands); this.#tick(); @@ -742,6 +745,12 @@ export default class RedisClient< return results; } + #addMultiCommands(commands: Array, chainId?: symbol) { + return Promise.all( + commands.map(({ args }) => this.#queue.addCommand(args, { chainId })) + ); + } + async* scanIterator(options?: ScanCommandOptions): AsyncIterable { let cursor = 0; do { diff --git a/packages/client/lib/client/multi-command.ts b/packages/client/lib/client/multi-command.ts index 749ab4c4e0..e347667bf2 100644 --- a/packages/client/lib/client/multi-command.ts +++ b/packages/client/lib/client/multi-command.ts @@ -170,12 +170,9 @@ export default class RedisClientMultiCommand { return this.execAsPipeline(); } - const commands = this.#multi.exec(); - if (!commands) return []; - return this.#multi.handleExecReplies( await this.#executor( - commands, + this.#multi.queue, this.#selectedDB, RedisMultiCommand.generateChainId() ) @@ -185,6 +182,8 @@ export default class RedisClientMultiCommand { EXEC = this.exec; async execAsPipeline(): Promise> { + if (this.#multi.queue.length === 0) return []; + return this.#multi.transformReplies( await this.#executor( this.#multi.queue, diff --git a/packages/client/lib/cluster/multi-command.ts b/packages/client/lib/cluster/multi-command.ts index 52ab6eb0e2..ef3c7590ec 100644 --- a/packages/client/lib/cluster/multi-command.ts +++ b/packages/client/lib/cluster/multi-command.ts @@ -120,11 +120,8 @@ export default class RedisClusterMultiCommand { return this.execAsPipeline(); } - const commands = this.#multi.exec(); - if (!commands) return []; - return this.#multi.handleExecReplies( - await this.#executor(commands, this.#firstKey, RedisMultiCommand.generateChainId()) + await this.#executor(this.#multi.queue, this.#firstKey, RedisMultiCommand.generateChainId()) ); } diff --git a/packages/client/lib/multi-command.spec.ts b/packages/client/lib/multi-command.spec.ts index 7e9667cd51..b0f79c6e15 100644 --- a/packages/client/lib/multi-command.spec.ts +++ b/packages/client/lib/multi-command.spec.ts @@ -46,24 +46,23 @@ describe('Multi Command', () => { }); describe('exec', () => { - it('undefined', () => { - assert.equal( - new RedisMultiCommand().exec(), - undefined + it('without commands', () => { + assert.deepEqual( + new RedisMultiCommand().queue, + [] ); }); - it('Array', () => { + it('with commands', () => { const multi = new RedisMultiCommand(); multi.addCommand(['PING']); assert.deepEqual( - multi.exec(), - [ - { args: ['MULTI'] }, - { args: ['PING'], transformReply: undefined }, - { args: ['EXEC'] } - ] + multi.queue, + [{ + args: ['PING'], + transformReply: undefined + }] ); }); }); diff --git a/packages/client/lib/multi-command.ts b/packages/client/lib/multi-command.ts index 8865cc8e00..08f23ffa45 100644 --- a/packages/client/lib/multi-command.ts +++ b/packages/client/lib/multi-command.ts @@ -69,18 +69,6 @@ export default class RedisMultiCommand { return transformedArguments; } - exec(): undefined | Array { - if (!this.queue.length) { - return; - } - - return [ - { args: ['MULTI'] }, - ...this.queue, - { args: ['EXEC'] } - ]; - } - handleExecReplies(rawReplies: Array): Array { const execReply = rawReplies[rawReplies.length - 1] as (null | Array); if (execReply === null) {