fastify / fastify-rate-limit

A low overhead rate limiter for your routes
MIT License
504 stars 71 forks source link

Move Redis logic to Redis-Side #330

Closed gurgunday closed 1 year ago

gurgunday commented 1 year ago

@Uzlopak asks, we implement āœŠ

The obvious benefit is that we now make 1 request per function call

Not sure what I think about using Lua as a JS enjoyer though ā€” arrays are 1-indexed šŸ¤¢

gurgunday commented 1 year ago

Found a nasty bug

Uzlopak commented 1 year ago

Please add an example example-auto-pipeline.js (copy of example.js) with autoPipelining set to true.

Add documentation regarding autoPipelining.

gurgunday commented 1 year ago

None of the examples are working anymore btw, they need to await fastify.register

Uzlopak commented 1 year ago

@gurgunday Thank you. I was currently testing your PR and was wondering why the rate-limitting is not working.

climba03003 commented 1 year ago

Thank you. I was currently testing your PR and was wondering why the rate-limitting is not working.

Obvious reason, using onRoute hook.

Uzlopak commented 1 year ago

Yeah..

Uzlopak commented 1 year ago

I asked in discord server of redis, if they have some recommendations regarding performance or best practices regarding this PR.

https://discord.com/channels/697882427875393627/865653073531633665

Lets wait if we get some answers.

airhorns commented 1 year ago

Just chiming in from afar, I think this is a major improvement, in my experience with redis in the past executing in scripts is far far faster than roundtripping for each element. The only downsides are you need a bit of complexity in the client to load the script if it hasn't been loaded already (which ioredis handles just fine), and you need to use a redis version with lua support, and it's been out for a long time. +1 from me on the direction of this PR. Do the existing tests cover this already?

gurgunday commented 1 year ago

@airhorns hey, thanks a lot for the comment

We do have tests that check if Redis behavior is correct

GitHub actions by default uses the latest version and Iā€™m not sure if we test for different redis versions, but this release will most definitely be a major bump

Uzlopak commented 1 year ago

I dont think that we will get some feedback from redis discord.

So i am om with this PR.