fastify / fastify-rate-limit

A low overhead rate limiter for your routes
MIT License
480 stars 67 forks source link

Align stores #323

Closed gurgunday closed 10 months ago

gurgunday commented 10 months ago

Fixes #304 and closes #321

Motivation: the implementations of the LocalStore and the RedisStore are doing different things, which is inconsistent and confusing users.

For instance, when continueExceeding is true, RedisStore always resets the TTL of an IP, even when it hasn't yet exceeded the limit. This causes people get rate limited more than necessary because for every request that hasn't exceeded the max, we keep on resetting the ttl instead of letting it go down, while also incrementing the counter.

This PR makes RedisStore mirror LocalStore's behavior. We increment the counter but do not reset the TLL unless the limit is exceeded, after which the TTL is reset for every request as long as the key is alive.

Uzlopak commented 10 months ago

@gurgunday

You hav some merge conflicts.

To be honest i dont like the ['0', '1', '2'].includes assertion. Before it was maybe flaky, and we maybe need to run the tests sequentially. But it would give use feedback on how reliant the rate-limitting is. Now it can be potentially unreliably and we could be blind to that.

gurgunday commented 10 months ago

Here are a few things we can do in terms of tests:

The first check of Retry-After or x-ratelimit nearly always returns the timeWindow, but for the rest, there just isn't a single value that we can pinpoint

If timeWindow is 3000, the first call is fine, 3000, but the second can return 2700, which will be floored to 2, and sometimes there are 800ms clock ticks too after which could it could take 210ms to create another Date and now it would be floored to 0

I was thinking, before we even fix these, that Math.ceil might be more appropriate than .floor in terms of rateLimit timers

Because really a client shouldn't see a TTL of 600ms floored down to 0 seconds, it's more useful if they see 1

gurgunday commented 10 months ago

Using .ceil seems to actually work much more reliably as well

Uzlopak commented 10 months ago

Floor is obviously more restricitive than Ceil.