feat: Add command timeout #2981

florian-schunk · 2025-05-28T10:30:38Z

Description

This PR introduces a timeout for commands sent to redis.
Our use-case is that we are using node-rate-limiter-flexible with node-redis, but sometimes the connection to redis is slow. In these cases we want to fall back the the insurance limiter of node-rate-limiter-flexible.

This feature was also requested in #2175

Checklist

Does npm test pass with this change (including linting)?
Is the new or changed code fully tested?
Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?

florian-schunk · 2025-06-18T15:58:41Z

Hi @nkaradzhov sorry for pinging you directly, but I couldn't find information on how to contact maintainers for this project.
What should we do for getting this PR reviewed and merged?

nkaradzhov · 2025-06-18T17:08:17Z

@florian-schunk hi, sorry for the delay, been a bit busy. I will take a look at this.

nkaradzhov · 2025-06-19T07:24:28Z

packages/client/lib/client/index.ts

+    let controller: AbortController;
+    if (this._self.#options?.commandTimeout) {
+      controller = new AbortController()
+      let abortSignal = controller.signal;
+      if (options?.abortSignal) {
+        abortSignal = AbortSignal.any([
+          abortSignal,
+	  options.abortSignal
+	]);
+      }
+      options = {
+        ...options,
+        abortSignal
+      }
+    }


We can simplify this by using the AbortSignal.timeout static method:

if (this._self.#options?.commandTimeout) { let abortSignal = AbortSignal.timeout( this._self.#options?.commandTimeout ); if (options?.abortSignal) { abortSignal = AbortSignal.any([abortSignal, options.abortSignal]); } options = { ...options, abortSignal }; }

I tried this, also together with the change in the test that you proposed, but somehow for me the test keeps failing with this approach. I am not quite understanding, why.

nkaradzhov · 2025-06-19T07:25:03Z

packages/client/lib/client/index.ts

+
    this._self.#scheduleWrite();
-    return promise;
+    if (!this._self.#options?.commandTimeout) {
+      return promise;
+    }
+
+    return new Promise<T>((resolve, reject) => {
+      const timeoutId = setTimeout(() => {
+        controller.abort();
+        reject(new CommandTimeoutError());
+      }, this._self.#options?.commandTimeout)
+      promise.then(result => {
+        clearInterval(timeoutId);
+        resolve(result)
+      }).catch(error => {
+        clearInterval(timeoutId);
+        reject(error)
+      });
+    })


Then, this change becomes obsolete

nkaradzhov · 2025-06-19T07:25:54Z

packages/client/lib/errors.ts

+export class CommandTimeoutError extends Error {
+  constructor() {
+    super('Command timeout');
+  }
+}
+


No need to introduce another error, we can use the AbortError

Ok, I changed it to use AbortError, but couldn't there be situation where it makes sense to distinguish between a timeout and having the command manually aborted? Wouldn't it then make more sense to have a different error?

nkaradzhov · 2025-06-19T07:29:01Z

packages/client/lib/client/index.spec.ts

+  testUtils.testWithClient('CommandTimeoutError', async client => {
+    const promise = assert.rejects(client.sendCommand(['PING']), CommandTimeoutError),
+      start = process.hrtime.bigint();
+
+    while (process.hrtime.bigint() - start < 50_000_000) {
+      // block the event loop for 1ms, to make sure the connection will timeout
+    }
+
+    await promise;
+  }, {
+    ...GLOBAL.SERVERS.OPEN,
+    clientOptions: {
+      commandTimeout: 50,
+    }
+  });
+


This test would need to change like this:

testUtils.testWithClient('CommandTimeoutError', async client => { const promise = client.sendCommand(['PING']) const start = process.hrtime.bigint(); while (process.hrtime.bigint() - start < 50_000_000) { // block the event loop for 50ms, to make sure the connection will timeout } assert.rejects(promise, AbortError); }, { ...GLOBAL.SERVERS.OPEN, clientOptions: { commandTimeout: 50, } });

nkaradzhov · 2025-06-20T15:54:28Z

I will take a look, maybe will make a pr to your pr

…

On Fri, Jun 20, 2025, 6:16 PM florian-schunk ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In packages/client/lib/client/index.ts <#2981 (comment)>: > + let controller: AbortController; + if (this._self.#options?.commandTimeout) { + controller = new AbortController() + let abortSignal = controller.signal; + if (options?.abortSignal) { + abortSignal = AbortSignal.any([ + abortSignal, + options.abortSignal + ]); + } + options = { + ...options, + abortSignal + } + } I tried this, also together with the change in the test that you proposed, but somehow for me the test keeps failing with this approach. I am not quite understanding, why. — Reply to this email directly, view it on GitHub <#2981 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AALIHLDULWEBI6J3YYY37E33EQQUVAVCNFSM6AAAAAB6CNSAFSVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDSNBWGU2DKMRQGE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

nkaradzhov · 2025-06-23T11:29:49Z

@florian-schunk i made a PR to your fork

use AbortSignal utilities instead of setTimeout

florian-schunk · 2025-06-25T08:27:47Z

packages/client/lib/client/index.spec.ts

+
+  testUtils.testWithClient('rejects with AbortError on commandTimeout timer', async client => {
+    const start = process.hrtime.bigint();
+    const promise = client.ping();


Just for my understanding: What is the difference between sendCommand(['PING'] and ping()?

not much. sendCommand is a catch-all command that can send any command. Usually, we would have all the commands available as dedicated methods, like client.ping(), client.hset() etc. But sometimes there are commands that are not yet implemented as a dedicated method. In those cases, users can fallback to using sendCommand

One significant difference is that sendCommand takes additional options. In the options, there is the abortSignal. So in practice, anyone who needs to be able to timeout a command can use sendCommand with abortSignal to achieve this.

nkaradzhov

CLGTM

nkaradzhov · 2025-06-26T08:46:05Z

@florian-schunk, I just spoke with our architect and he suggested we also test if this works properly with cluster and sentinel. While looking into this, i understood that we are not exposing this parameter in the cluster config and sentinel config. So it will need a bit more work. Let me take this for now and I will make a PR to your fork once im ready. It will take some time as I will not be available next week.

further changes needed

florian-schunk force-pushed the addCommandTimeout branch 3 times, most recently from 4ad6a4a to 90f8fe9 Compare June 18, 2025 15:20

florian-schunk added 3 commits June 18, 2025 16:25

add command timeout

4b89ef9

update documentation

0b98f24

pass abort signal

e8eb95b

florian-schunk force-pushed the addCommandTimeout branch from 90f8fe9 to e8eb95b Compare June 18, 2025 15:27

add node types

19e4935

nkaradzhov requested changes Jun 19, 2025

View reviewed changes

nkaradzhov mentioned this pull request Jun 20, 2025

feat: added commandTimeout option #2926

Closed

3 tasks

reuse AbortError

dd72fea

nkaradzhov and others added 2 commits June 23, 2025 12:19

use AbortSignal utilities instead of setTimeout

3c9b0fc

Merge branch 'addCommandTimeout' into command-timeout

0d16695

Merge pull request #1 from nkaradzhov/command-timeout

f2a3076

use AbortSignal utilities instead of setTimeout

florian-schunk commented Jun 25, 2025

View reviewed changes

nkaradzhov previously approved these changes Jun 25, 2025

View reviewed changes

nkaradzhov requested a review from bobymicroby June 25, 2025 13:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat: Add command timeout #2981

feat: Add command timeout #2981

Uh oh!

florian-schunk commented May 28, 2025 •

edited

Loading

Uh oh!

florian-schunk commented Jun 18, 2025

Uh oh!

nkaradzhov commented Jun 18, 2025

Uh oh!

nkaradzhov Jun 19, 2025

Uh oh!

florian-schunk Jun 20, 2025

Uh oh!

nkaradzhov Jun 19, 2025

Uh oh!

nkaradzhov Jun 19, 2025

Uh oh!

florian-schunk Jun 20, 2025

Uh oh!

nkaradzhov Jun 19, 2025

Uh oh!

nkaradzhov commented Jun 20, 2025 via email

Uh oh!

nkaradzhov commented Jun 23, 2025

Uh oh!

florian-schunk Jun 25, 2025

Uh oh!

nkaradzhov Jun 25, 2025

Uh oh!

nkaradzhov left a comment

Uh oh!

nkaradzhov commented Jun 26, 2025

Uh oh!

Uh oh!

feat: Add command timeout #2981

Are you sure you want to change the base?

feat: Add command timeout #2981

Uh oh!

Conversation

florian-schunk commented May 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

florian-schunk commented Jun 18, 2025

Uh oh!

nkaradzhov commented Jun 18, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nkaradzhov commented Jun 20, 2025 via email

Uh oh!

nkaradzhov commented Jun 23, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nkaradzhov left a comment

Choose a reason for hiding this comment

Uh oh!

nkaradzhov commented Jun 26, 2025

Uh oh!

Uh oh!

florian-schunk commented May 28, 2025 •

edited

Loading