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
- Restore int64 based atomic rate limiter (#94) This limiter was introduced and merged in the following PR #85 Later @twelsh-aw found an issue with this implementation #90 So @rabbbit reverted this c... — committed to uber-go/ratelimit by storozhukBM 2 years ago
- Restore int64 based atomic rate limiter (#94) This limiter was introduced and merged in the following PR #85 Later @twelsh-aw found an issue with this implementation #90 So @rabbbit reverted this cha... — committed to storozhukBM/ratelimit by storozhukBM 2 years ago
- Update example to include a withoutSlack option In #90 @twelsh-aw found a bug in a new implementation. This turned out to be caused by us mocking time. Since time mocking will always have some risks... — committed to uber-go/ratelimit by rabbbit 2 years ago
- Update example to include a withoutSlack option (#96) In #90 @twelsh-aw found a bug in a new implementation. This turned out to be caused by us mocking time. Since time mocking will always have s... — committed to uber-go/ratelimit by rabbbit 2 years ago
- Fix no slack option for int64 based option (#95) This PR fixes the issue found by @twelsh-aw with int64 based implementation #90 Our tests did not detect this issue, so we have a separate PR #93 t... — committed to uber-go/ratelimit by storozhukBM 2 years ago
- Make int64 based atomic ratelimiter default (#101) * Fix return timestamp discrepancy between regular atomic limiter and int64 based one * Make int64 based atomic limiter default Long story: this... — committed to uber-go/ratelimit by storozhukBM 2 years ago
- Update clock dependency There were quite a few fixes in that repo, let's pull them in. We've had a few mock-clock bugs before (#90), I'm hoping that with the new versions #93 won't be necessary. I'l... — committed to uber-go/ratelimit by rabbbit a year ago
@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.