-
-
Notifications
You must be signed in to change notification settings - Fork 33
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
PromiseRejectionHandledWarning: Promise rejection was handled asynchronously #190
Comments
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 :( |
@abhijeetviswa Could you try out the fix I pushed for this? I think the following commands should help you install the fix locally:
(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 |
@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:
Any suggestions on how to ignore any redis issues during a request and just continue? |
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? |
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?) |
The redis server version is 7.0.7 so it's most definitely just a flaky Redis connection issue from my side. With all that being said, we should still handle the |
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. |
That's fine. FYI, by making sure that the redis connection is established before constructing an instance |
Oh, that makes sense! Glad it's working now. |
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:
Let me know what you think. Regards, |
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 |
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. |
What about errors during the construction of the |
That is done while passing options to the rate limiter, so that will throw an error, not pass it to the error handler. |
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 |
Description
I get the following error the first time a request comes in:
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
The text was updated successfully, but these errors were encountered: