Closed gurgunday closed 1 year ago
Yeah, should've tested Resis as well, will take a second
@kibertoad
Do you agree with using toad-cache
. I know the maintainer of toad-cache, and this guy seems shady.
@Uzlopak I only trust him as far as I can throw him, so no. We need more respectable members of the community in that team for increased confidence, so hopefully we can get some.
(sent invites to you and @gurgunday just in case you are open to it, feel free to decline if not)
:D
Thank you
Thanks @kibertoad! 🙏🚀
Alright, you’re right, I’ll break them up 🙏
But I think the refactor is important and should land in one way or another
Rate limiting is a crucial part of a web application, so we must constantly strive to improve
fastify-rate-limit
's performance and architectureBefore doing some more significant improvements, I wanted to simplify the codebase so that future changes don't get mixed up with these
This PR results in some performance gain thanks to the following changes:
use
toad-cache
Toad cache is faster than
tiny-lru
in important benchmarks and I've shared my interest with kibertoad on how we can centralize all LRU and FIFO packages to his within Fastifyuse our own TTL
I realized while looking at the source code that we already store an
iterationStartTime
usingDate.now()
, we are however not doing enough with this information and just returning it back to user. We can increase performance by disabling the TTL in the LRU (since it would otherwise have moreDate.now
calls) and reseting a request lazily ourselvesmove objects and functions out of the main function
Since
rate-limit
supports being registered multiple times, every time a user does that, we are risking recreating the objects that are inside the main function, which can lead to increased memory usagechange logic order
In places like the onRequest handler, we would do calculations before checking for conditions that would get us out of the logic flow, for instance we would parse the timeWindow string using
ms
before checking if the hook had already runbug fixes
master
as well) was fixedallowList
would get ignored if the globalallowList
property was a functiontimeWindow
property, ex:fastify.rateLimit({max: 5})
, the options would not get registeredI've added tests for the bug fixes
Benchmark (new vs. old, highest of 5 runs):