Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Correctly check whether the time is in the past #13

Merged
merged 1 commit into from Feb 28, 2018

Conversation

javitonino
Copy link
Contributor

Previous code checked if the time was less than 1 second in the future, which produces incorrect results when the period between allowed requests is < 1 second (rate > 1/s).

Closes #12

Previous code checked if the time was less than 1 second
in the future, which produces incorrect results when the period
between allowed requests is < 1 second (rate > 1/s).
@javitonino
Copy link
Contributor Author

Test script:

require "redis"

r = Redis.new(port: 9000)
num_allowed = 0
num_blocked = 0

start = Time.now
begin
  1000.times do
    resp = r.call("CL.THROTTLE", "user#{start}", "10", "100", 1, 1)
    if resp[0] == 0
      num_allowed += 1
    else
      num_blocked += 1
      sleep 0.1
    end
  end
ensure
  elapsed = Time.now - start
  p "Allowed: #{num_allowed}"
  p "Blocked: #{num_blocked}"
  p "Elapsed: #{elapsed} s"
  p "Rate: #{num_allowed/elapsed}"
end

I expect a rate ~100 req/s (a bit higher, because of the initial burst).

Before this PR:

"Allowed: 466"
"Blocked: 34"
"Elapsed: 3.564658991 s"
"Rate: 130.72779224507875"

After this PR:

"Allowed: 366"
"Blocked: 35"
"Elapsed: 3.661400648 s"
"Rate: 99.96174556857729"

@brandur
Copy link
Owner

brandur commented Feb 28, 2018

Wow, amazing work tracking this one down. Thanks @javitonino and all!

I'd forgotten that I'd written this, but looking at it now, I can't believe I didn't catch this at the time. It's quite a glaring hole in retrospect.

@brandur brandur merged commit 88478b7 into brandur:master Feb 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants