PromiseRejectionHandledWarning: Promise rejection was handled asynchronously

[abhijeetviswa]

Description

I get the following error the first time a request comes in:

(node:66707) PromiseRejectionHandledWarning: Promise rejection was handled asynchronously (rejection id: 4)
    at handledRejection (node:internal/process/promises:172:23)
    at promiseRejectHandler (node:internal/process/promises:118:7)
    at evalCommand (...)
    at RedisStore.runCommandWithRetry (...)
    at RedisStore.increment (...)
    at processTicksAndRejections (node:internal/process/task_queues:95:5)
    at async node_modules/express-rate-limit/dist/index.cjs:553:5

Possibly because of a missing await here: https://github.com/express-rate-limit/rate-limit-redis/blob/f9b187878a554a89495c349e43a31af17abcb731/source/lib.ts#L110C14-L110C14

Library version

^6.10.0

Node version

v18.12.1

Typescript version (if you are using it)

5.0.4

Module system

CommonJS

[nfriedly]

You might be right. It's annoying that it doesn't include the line number for evalCommand in the stack trace :/

The root cause is probably some other issue, but this async thing is causing the error to be swallowed :(

[gamemaker1]

@abhijeetviswa Could you try out the fix I pushed for this?

I think the following commands should help you install the fix locally:

cd ~
git clone https://github.com/express-rate-limit/rate-limit-redis.git
cd rate-limit-redis
git checkout fix-retry-error
npm install
npm run compile
npm link
cd ~/project/dir
npm link rate-limit-redis

(The npm link command lets you use a local version of a dependency instead of the one installed from npm - see docs.npmjs.com/cli/v9/commands/npm-link.)

Once you're done testing the fix, a regular npm install rate-limit-redis should undo the link.

[abhijeetviswa]

@gamemaker1 Tried this out. Getting the same warning again. Though that might have to do with an error handler defined somewhere in this project or my project.

The second call from your catch block is propgated to my express error handler:

ClientClosedError: The client is closed
    at Commander._RedisClient_sendCommand (/Users/abhijeet/workspace/***i/node_modules/@redis/client/dist/lib/client/index.js:490:31)
    at Commander.sendCommand (/Users/abhijeet/workspace/***/node_modules/@redis/client/dist/lib/client/index.js:191:100)
    at RedisStore.sendCommand (/Users/abhijeet/workspace/***i/src/middlewares/rate-limit.ts:25:28)
    at RedisStore.loadScript (/Users/abhijeet/workspace/rate-limit-redis/dist/index.cjs:37:31)
    at new RedisStore (/Users/abhijeet/workspace/rate-limit-redis/dist/index.cjs:34:34)
    at getStore (/Users/abhijeet/workspace/***/src/middlewares/rate-limit.ts:23:9)
    at Object.<anonymous> (/Users/abhijeet/workspace/***/src/middlewares/rate-limit.ts:38:9)
    at Module._compile (node:internal/modules/cjs/loader:1159:14)
    at Module.m._compile (/Users/abhijeet/workspace/***/node_modules/ts-node/src/index.ts:1618:23)
    at Module._extensions..js (node:internal/modules/cjs/loader:1213:10)

Any suggestions on how to ignore any redis issues during a request and just continue?

[nfriedly]

That looks like the SCRIPT LOAD command is failing - do some versions of redis not support that?

Maybe try with an older version of rate-limit-redis (2.x, I think) and see if it works?

[nfriedly]

That looks like the SCRIPT LOAD command is failing - do some versions of redis not support that?

Looks like the answer is yes. The command has been supported since redis 2.6.0 (which was released quite a while ago), but it's possible to disable it via ACL or renaming it to "", and some redis instances do this a s a security precaution.

We should probably support a fallback to sending a muti-command, or else just go back to doing that by default. (Why did we switch to the script version in the first place?)

[abhijeetviswa]

The redis server version is 7.0.7 so it's most definitely just a flaky Redis connection issue from my side.
I don't have problems with this. Though, any suggestions on how to use a fallback store with express-rate-limit in such cases would be helpful.

With all that being said, we should still handle the PromiseRejectionHandledWarning since that may become an error in the future.

[nfriedly]

Though, any suggestions on how to use a fallback store with express-rate-limit in such cases would be helpful.

Okay I'm sorry, I don't think it's possible today; I was saying we need to make some changes.

You're right that we need to handle the promise rejection properly.

[abhijeetviswa]

Okay I'm sorry, I don't think it's possible today

That's fine.

FYI, by making sure that the redis connection is established before constructing an instance RedisStore got rid of the warning. The lack of a connection during construction was the issue all along for me.

[nfriedly]

FYI, by making sure that the redis connection is established before constructing an instance RedisStore got rid of the warning. The lack of a connection during construction was the issue all along for me.

Oh, that makes sense! Glad it's working now.

[gamemaker1]

Hi,

I'm sorry I missed the discussion while it was happening; also glad to know that you were able to fix it.

I think we should do either (or both?) of the following:

• Implement a retry function that uses exponential backoff to keep trying based on the error returned by redis, and logs a warning in the console everytime it retries.
• Re-add the passOnConnectionError option, and let the request succeed as a failover in case a connection error occurs.

Let me know what you think.

Regards,
Vedant

[abhijeetviswa]

Re-add the passOnConnectionError option, and let the request succeed as a failover in case a connection error occurs.

I'm actually looking for something similar since the rate-limiter is a non-critical, good to have component. Wouldn't want my app to crash if redis fails.

I'd prefer the above + warning messages or an outright error so that the user can handle it. Not sure if propagating errors to the user is possible with the current setup of express-rate-limit.

[gamemaker1]

Great! I'll work on a PR for the same.

Also, I think it should be possible to propagate an error back, any error thrown within the middleware should be passed to the error handler defined for express.

[abhijeetviswa]

What about errors during the construction of the RedisStore instance itself?

[gamemaker1]

That is done while passing options to the rate limiter, so that will throw an error, not pass it to the error handler.

[v11t]

Okay I'm sorry, I don't think it's possible today

That's fine.

FYI, by making sure that the redis connection is established before constructing an instance RedisStore got rid of the warning. The lack of a connection during construction was the issue all along for me.

Thanks, this helped fix the odd 504s when the redis store was used to rate limit a cluster behind a load balancer.

Here's a snippet for the reference:

import { promisify } from "util";
import { rateLimit } from "express-rate-limit";
import RedisStore from "rate-limit-redis";
// your client config, something like const redisClient = new RedisClient(...);
import { redisClient } from "../redis";

let store: RedisStore | undefined = undefined;

async function initializeRateLimitStore() {
  try {
    const ping = promisify(redisClient.ping).bind(redisClient);
    await ping();
    store = new RedisStore({
      // @ts-expect-error - Known issue: the `call` function is not present in @types/ioredis
      sendCommand: (...args: string[]) => redisClient.call(...args),
    });
  } catch (err) {
    // log and handle
  }
}

(async () => {
  await initializeRateLimitStore();
})();

const redisLimit = rateLimit({
  store,
  // rest of your init params ...
})

versions: rate-limit-redis@^4.0.0, express-rate-limit@^7.0.1