ejfinneran / ratelimit

A Redis-backed rate limiter written in Ruby
MIT License
260 stars 57 forks source link

Wrong count result after long inactivity #2

Closed alno closed 13 years ago

alno commented 13 years ago

Consider following test:

  should "be able to add to the count for a given subject" do
    @r.add("value1")
    @r.add("value1")
    assert_equal 2, @r.count("value1", 1)
    assert_equal 0, @r.count("value2", 1)
    Timecop.travel(600) do
      assert_equal 0, @r.count("value1", 1)
    end
  end

It fails.

After 600 seconds of inactivity bucket wasn't expired yet, but it's index is the same again, so it return wrong result in count. Although it's rare case, it's possible to produce wrong results. Also, probability of error increases when increasing interval in count.

One of possible solutions may be restricting bucket_expire to be no more than bucket_span minus maximum interval which may be requested in count.

ejfinneran commented 13 years ago

Good catch. I'm trying to think of a reason we can't just set bucket_expiry to the same value as bucket_span by default and raise an exception if the user tries to set it to larger than bucket_span. Thoughts?

https://github.com/ejfinneran/ratelimit/commit/b5b81dcc488f231b6a7abbdeb87c7ddf247b4029

alno commented 13 years ago

Hmm.. Yes, i think it solves problem. It was my error in previous post - no need to substract maximum interval because of buckets in it are counted back, not forward.