Skip to content
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

RedisStore is required with each limiter #171

Closed
Zirafnik opened this issue Jul 11, 2023 · 4 comments
Closed

RedisStore is required with each limiter #171

Zirafnik opened this issue Jul 11, 2023 · 4 comments

Comments

@Zirafnik
Copy link

Zirafnik commented Jul 11, 2023

Description

This not an bug, but just an issue I faced, which will hopefully save somebody hours of work.

For most this is probably already obvious, but you need to create a new RedisStore for each limiter.

You cannot define a RedisStore and then re-use it accross all limiters. This does NOT work:

const defaultSettings = {
    standardHeaders: true,
    legacyHeaders: false,
    store: new RedisStore({
            sendCommand: (...args: string[]) => client.sendCommand(args),
            prefix: 'rateLimit:',
    }),
};

const limiterOne = rateLimit({
    ...defaultSettings,
    windowMs: 24 * 60 * 60 * 1000, // 24 hours
    max: 500, 
});


const limiterTwo = rateLimit({
    ...defaultSettings,
    windowMs: 60 * 1000, // 1 min
    max: 10, 
});

It doesn't work, because RedisStore reads the windowMs property when initializing, and uses it to set reset-timer. Without it, the reset-timer is set by default to 24 hours. Additionally, in the above example the RedisStore would always use the same key prefix, which means that different API routes would count towards the same limit.

So in short, you always need to create a new RedisStore with each limiter.

const defaultSettings = {
    standardHeaders: true,
    legacyHeaders: false,
};

const limiterOne = rateLimit({
    ...defaultSettings,
    windowMs: 24 * 60 * 60 * 1000, // 24 hours
    max: 500, 
    store: new RedisStore({
            sendCommand: (...args: string[]) => client.sendCommand(args),
            prefix: 'rateLimitOne:',
    }),
});


const limiterTwo = rateLimit({
    ...defaultSettings,
    windowMs: 60 * 1000, // 1 min
    max: 10, 
    store: new RedisStore({
            sendCommand: (...args: string[]) => client.sendCommand(args),
            prefix: 'rateLimitTwo:',
    }),

EXTRA

You can extract the sendCommand function to re-use it across all new RedisStores.

const defaultSettings = {
    standardHeaders: true,
    legacyHeaders: false,
};

const sendCommand = (...args) => client.sendCommand(args);

const limiterOne = rateLimit({
    ...defaultSettings,
    windowMs: 24 * 60 * 60 * 1000, // 24 hours
    max: 500, 
    store: new RedisStore({
            sendCommand,
            prefix: 'rateLimitOne:',
    }),
});


const limiterTwo = rateLimit({
    ...defaultSettings,
    windowMs: 60 * 1000, // 1 min
    max: 10, 
    store: new RedisStore({
            sendCommand,
            prefix: 'rateLimitTwo:',
    }),
@nfriedly
Copy link
Member

express-rate-limit/express-rate-limit#364 should help reduce the confusion here. It will make it log an error if the first request is counted more than once with the same store/key/prefix. The error will have a link to https://express-rate-limit.github.io/ERR_ERL_DOUBLE_COUNT/ where we can put a bit of additional information.

@shrihari-prakash
Copy link

I'm sorry if the question sounds silly, but I'm gonna ask this anyway. I understand that it is not a good practice to use same store for multiple rate limiter instances, but what happens when I am running multiple instances of my application for scaling reasons? Can they not share the same redis instance (the same prefix)? If not, how can rate limits be counted without reset if say the http requests were routed to different instances of my application in a round robin fashion? @Zirafnik @nfriedly

@nfriedly
Copy link
Member

Sorry @shrihari-prakash , I think the word "instances" makes this more confusing.

  • If you have multiple instances of your app, via multiple servers or even multiple processes on the same server (e.g. via the cluster module), they should all be configured to use the same redis instance with the same prefix.
  • If you have multiple instances of express-rate-limit within a single instance of your app (e.g. to have different rate limits for different urls, or to have higher limit per day and a lower limit per hour), those instances of express-rate-limit should each use a different prefix.

Does that make more sense?

Another way to put it is that each time you write rateLimit({...}) in your code, you should come up with a new prefix. But that code can be deployed and run from multiple places without the prefix changing.

@shrihari-prakash
Copy link

shrihari-prakash commented Mar 1, 2025

@nfriedly that makes perfect sense :). I was more concerned about scenario 1 that you mentioned.
For me, the line

if multiple instances are called that use the same Store

in this page raised a concern because it does not mention if it is inside the same process or across different processes. Thank you for clarifying. It would also be fantastic if the docs reflected that as well!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants