ejfinneran / ratelimit

A Redis-backed rate limiter written in Ruby
MIT License
257 stars 55 forks source link

Use absolute bucket numbers instead of a wrapping set of buckets #46

Closed phillipp closed 8 months ago

phillipp commented 1 year ago

This fixes #45 and improves performance by calculcating the count for subjects in redis by lua scripts without transferring the values of the buckets

An additional maintenance call is necessary to cleanup old keys in a subjects hash. If the subject is not accessed for a longer time it will still be removed by the existing expiration.

phillipp commented 1 year ago

Obviously the problem is that fakeredis does not implement lua scripting via script.

I would suggest to remove fakeredis, would that be okay?

andreasknoepfle commented 1 year ago

I guess if we can't avoid it then it is fine for sure. We anyway have to fix the CI of this project at some time and migrate to github actions, where we might just be able to run redis: https://docs.github.com/en/actions/using-containerized-services/creating-redis-service-containers

Locally I think we can assume redis to be around when running tests. We should just make sure that we clean all the redis state before when we do not use fakeredis, so we can consistently run test.

An alternative could be to monkeypatch the script behaviour into fakeedis as test support: https://github.com/guilleiguaran/fakeredis/issues/94

phillipp commented 1 year ago

@andreasknoepfle I have updated the pull request and removed fakeredis, all tests pass. The specs already issue flushdb to remove the redis database contents.

kylejw2 commented 8 months ago

Is there a reason this PR hasn't been merged yet? I'm experiencing the same issue and I'm contemplating not using this gem any more since this hasn't been fixed and there's no good workaround

andreasknoepfle commented 8 months ago

@kylejw2 thanks for the reminder. I am not working on a lot of ruby projects lately and this slipped through my fingers.

andreasknoepfle commented 8 months ago

Released as v1.1.0

kylejw2 commented 8 months ago

Thank you @andreasknoepfle!

renatovico commented 3 weeks ago

this break me,

I setup Ratelimit with :

Ratelimit.new(rate_limit_key, {redis: Rails.cache.redis})

this implicit use the connection Pool

Backtrace:

undefined method `script' for #<ConnectionPool:0x00007f8729861b88 @size=5, @timeout=5, @auto_reload_after_fork=true, @available=#<ConnectionPool::TimedStack:0x00007f8729861a98 @create_block=#<Proc:0x00007f872b6a9938 /home/renato/.rbenv/versions/3.2.3/lib/ruby/gems/3.2.0/gems/activesupport-7.1.4/lib/active_support/cache/redis_cache_store.rb:153>, @created=1, @que=[#<Redis client v5.3.0 for redis://localhost:6379>], @max=5, @mutex=#<Thread::Mutex:0x00007f872b6a96e0>, @resource=#<Thread::ConditionVariable:0x00007f872b6a9438>, @shutdown_block=nil>, @key=:"pool-11400", @key_count=:"pool-11400-count"> /home/renato/.rbenv/versions/3.2.3/lib/ruby/gems/3.2.0/gems/ratelimit-1.1.0/lib/ratelimit.rb:165:in `load_scripts'
/home/renato/.rbenv/versions/3.2.3/lib/ruby/gems/3.2.0/gems/ratelimit-1.1.0/lib/ratelimit.rb:62:in `initialize'