ratelimit: atomicInt64Limiter WithoutSlack doesn't block

First off thanks for the library 😃

Saw that a new rate limiter was introduced that benchmarked a lot better and pulled it down to try it out.

Noticed that when running WithoutSlack, it just allows everything through instead of waiting because all subsequent Take() calls fall into the case now-timeOfNextPermissionIssue > int64(t.maxSlack)

Easiest way to repro is using your example_test.go:

  • Using rl := ratelimit.New(100) (slack=10):
go test -run Example -count=1
=== RUN   Example
--- PASS: Example (0.09s)
PASS
ok      command-line-arguments  0.207s
  • Using rl := ratelimit.New(100, ratelimit.WithoutSlack)
go test -run Example -count=1   
--- FAIL: Example (0.01s)
got:
1 10ms
2 775µs
3 3µs
4 2µs
5 10µs
6 2µs
7 2µs
8 2µs
9 2µs
want:
1 10ms
2 10ms
3 10ms
4 10ms
5 10ms
6 10ms
7 10ms
8 10ms
9 10ms
FAIL
FAIL    command-line-arguments  0.126s
FAIL
  • Using 0.2.0 rl :=newAtomicBased(100, WithoutSlack)
go test -run Example -count=1
PASS
ok      go.uber.org/ratelimit   0.323s

I am not 100% sure why the other units with mocked clocks are passing, but your example test and my application tests fail consistently with this new limiter. On darwin if that helps.

About this issue

  • Original URL
  • State: closed
  • Created 2 years ago
  • Comments: 18 (18 by maintainers)

Commits related to this issue

Most upvoted comments

@rabbbit agree, I’ll look at possible options with time mocking and will fix our tests in the following PR

@twelsh-aw Yes, the issue is definitely there, I have a quick fix for it, but I also want to spend a bit more time to figure out why our tests don’t catch that issue.