Fix immediate slack re-apply issue #120
Conversation
|
@rabbbit please take a look |
|
I'll take a look on Monday. I'm not a fan of the test, both sleeping and asserting that time passed. I'm also considering reverting to the basic locking version. This is the second bug we found in this one, so it's clearly beyond both of us to write this confidently. |
|
This bug causes the failure of the rate limiter when it's a single instance of an online service. For example, a client with intermittent requests to call another service. The client holds a rate limiter as a field. The target service sometimes returns 429 (too many requests). It took me a lot of time to realize that it was a problem this PR to fix, and I always thought it was something in the way I invoked PLZ move forward with merging this PR into the main ASAP. 🙏 🙏 🙏 There is a lack of docs about |
| @@ -69,7 +69,7 @@ func (t *atomicInt64Limiter) Take() time.Time { | |||
| case timeOfNextPermissionIssue == 0 || (t.maxSlack == 0 && now-timeOfNextPermissionIssue > int64(t.perRequest)): | |||
| // if this is our first call or t.maxSlack == 0 we need to shrink issue time to now | |||
| newTimeOfNextPermissionIssue = now | |||
| case t.maxSlack > 0 && now-timeOfNextPermissionIssue > int64(t.maxSlack): | |||
| case t.maxSlack > 0 && now-timeOfNextPermissionIssue > int64(t.maxSlack)+int64(t.perRequest): | |||
There was a problem hiding this comment.
Wouldn't this solution make the withSlack and withoutSlack options work exactly the same.
Looks like without slack you condition looks like:
now - prev > period
and withSlack
now - (prev - slack) > slack + period
Which is the same thing
There was a problem hiding this comment.
We apply now - int64(t.maxSlack) only under condition and that makes the difference.
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.
|
Sorry for the delay @storozhukBM - I believe I understand your fix, but propose we do something like #124 to get the test in-line. I remember about your clock implementation and I dislike where we are, but I'm not willing to give up on tests totally just yet :) |
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.
|
Fixed via #124 instead - thank you for the debugging/fix here @storozhukBM. |
This issue is reported here #119
I was able to reproduce and fix it.