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

Take always return without block if it enters the case branch #119

Closed
smallnest opened this issue Dec 1, 2023 · 3 comments · Fixed by #124
Closed

Take always return without block if it enters the case branch #119

smallnest opened this issue Dec 1, 2023 · 3 comments · Fixed by #124

Comments

@smallnest
Copy link

smallnest commented Dec 1, 2023

https://github.com/uber-go/ratelimit/blob/main/limiter_atomic_int64.go#L72

		case t.maxSlack > 0 && now-timeOfNextPermissionIssue > int64(t.maxSlack):
			// a lot of nanoseconds passed since the last Take call
			// we will limit max accumulated time to maxSlack
			newTimeOfNextPermissionIssue = now - int64(t.maxSlack)

you will find the case always (almost) returns true.

The below code is a test for reproduce:

func TestLimiter(t *testing.T) {
	limiter := ratelimit.New(1, ratelimit.Per(time.Second), ratelimit.WithSlack(1))

	for i := 0; i < 25; i++ {
		if i == 1 {
			time.Sleep(2 * time.Second)
		}
		limiter.Take()

		fmt.Println(time.Now().Unix(), i) // burst

	}
}

ratelimit is v0.3.0

@storozhukBM
Copy link
Contributor

storozhukBM commented Dec 1, 2023

@smallnest can you please try the version from #120 and confirm if it works for you

@smallnest
Copy link
Author

yes. Fixed. thanks

@spencercjh
Copy link

spencercjh commented Dec 11, 2023

The fixed PR isn't merged. @smallnest Please re-open the issue to notify the maintainer and other users to pay attention to it. It's a bug that leads to an incident in my prod service. 😢 😭

This issue is also associated with #106 .

rabbbit added a commit that referenced this issue Mar 4, 2024
Fixes #119
The solution is a copy from #120, but follows the testing framework
that we have - I did not want us to have a real `Sleep` in tests.

I'm not exactly thrilled by the testing setup (especially the
milliseconds) or the clock itself, but I'm not willing to totally give
up on it like #120 proposes.
I also wanted ALL implementations of the ratelimiter to be tested,
not just the currently selected.

Might follow up with some testing cleanups and/or clock migration.
rabbbit added a commit that referenced this issue Mar 4, 2024
Fixes #119
The solution is a copy from #120, but follows the testing framework
that we have - I did not want us to have a real `Sleep` in tests.

I'm not exactly thrilled by the testing setup (especially the
milliseconds) or the clock itself, but I'm not willing to totally give
up on it like #120 proposes.
I also wanted ALL implementations of the ratelimiter to be tested,
not just the currently selected.

Might follow up with some testing cleanups and/or clock migration.
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 a pull request may close this issue.

3 participants