atomicInt64Limiter WithoutSlack doesn't block

[twelsh-aw]

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.

[storozhukBM]

@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.

[rabbbit]

Reverting - @storozhukBM we can re-do the whole PR again (possibly add some tests earlier?)

[storozhukBM]

@rabbbit
I figured out that our tests continue to pass even if I break currently existing implementation.
For example if you change ratelimit_test.go:108 to t.clock.Sleep(interval-interval) tests will still pass.

[storozhukBM]

@rabbbit
Further findings. Tests do not able to detect issues because with mock clock, literally time, freezes between Take() calls, to when we check now.Sub(oldState.last) in atomicLimiter or when we check now-timeOfNextPermissionIssue in atomicInt64Limiter it always returns 0

[storozhukBM]

@rabbbit
If we add r.clock.Add(time.Microsecond) between takes in (r *runnerImpl) startTaking tests start to detect issues.
The problem with that is that we call startTaking in different goroutines and "github.com/benbjohnson/clock" is not concurrently safe, so race detector fails when it finds some races inside mockClock state.

So we either should find concurrent mock clock implementation, or fix "github.com/benbjohnson/clock" or write our own.

[rabbbit]

I'm sorry, I won't be able to look at this in detail, for probably next ~10 days (limited laptop access).

For now, I can revert the clock implementation change in the meantime if you think it's better/stable - the tests were (mostly?) stable there. I'm opposed to implementing a clock as part of this package :)

Looks like we'll be stuck with the old implementation for a while - "code freeze" until we fix the tests.

[storozhukBM]

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

[storozhukBM]

@rabbbit I created a new PR as discussed #93

[storozhukBM]

@rabbbit

PR with fix for this issue #95

Also you can see on tests approach PR that it is able to detect this bug

[rabbbit]

Ack. This is pretty high on my priority list, will try to get to this soon. Again, sorry for the delay.

[storozhukBM]

@rabbbit We already have a fix in place. What do you think about making atomicInt64Limiter a default implementation again and allowing people to have a try before cutting a new release tag? Or we can make this implementation public so that users can instantiate it. I'd like the new testing approach to be merged first, but if you're unsure about it, we can skip it for now.

[rabbbit]

The new atomic version seems to perform so much better than it feels we should enable it.

However, after that, we might just have to declare a code freeze until someone else has more cycles for reviews. Sadly this includes the new time-testing, would need to look at this in much more detail before merging in.

[storozhukBM]

@rabbbit OK, so should I make a PR now?

[rabbbit]

SGTM; also, thanks for pushing this through :)

[storozhukBM]

@rabbbit
Please take a look
#101

[rabbbit]

Closing as fixed - we reverted the bad change in #91, @storozhukBM fixed it in #95, but we were defaulting to the old implementation. #101 makes the new (faster) implementation the default.

[storozhukBM]

@twelsh-aw this change is now merged, can you please try again and give us your feedback

[storozhukBM]

thank you @rabbbit