search

fastify / fastify-rate-limit

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

fix RedisStore resetting the TTL unnecessarily and align its behavior with LocalStore #322

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.

This should land after #320

If you only want to see the changes of this PR, you can see my local PR: https://github.com/gurgunday/fastify-rate-limit/pull/2

I still don't know how to create a PR on another PR 🤠

gurgunday commented 10 months ago

I apologize for the mess, this is an important bug fix that is bundled with a moderately significant refactor of the plugin

I am confident the code is better than ever and the confidence comes from standing on the shoulders of 2K+ unit tests that were written by our amazing contributors

mcollina commented 10 months ago

Why?

gurgunday commented 10 months ago

I was asked to separate the PRs to make it easier for the reviewers, I will try to introduce the changes one by one