Fix no slack option for int64 based option #95

storozhukBM · 2022-07-02T19:54:40Z

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 that enhances our tests approach to detect potential errors better.

codecov · 2022-07-02T19:56:12Z

Codecov Report

Merging #95 (dc66526) into main (2cba897) will not change coverage.
The diff coverage is 100.00%.

❗ Current head dc66526 differs from pull request most recent head ef02693. Consider uploading reports for the commit ef02693 to get more accurate results

@@            Coverage Diff            @@
##              main       #95   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            4         4           
  Lines           97        95    -2     
=========================================
- Hits            97        95    -2

Impacted Files	Coverage Δ
limiter_atomic_int64.go	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2cba897...ef02693. Read the comment docs.

storozhukBM · 2022-07-02T21:01:43Z

No perf degradation detected due to this fix

rabbbit

I'm really sorry about delaying this.

This is pretty top on my priority list I'll really try to get this soon.

(will need to understand the testing/time changes too)

rabbbit · 2022-07-05T06:30:28Z

limiter_atomic_int64.go

@@ -66,10 +66,10 @@ func (t *atomicInt64Limiter) Take() time.Time {
 		timeOfNextPermissionIssue := atomic.LoadInt64(&t.state)

 		switch {
-		case timeOfNextPermissionIssue == 0:
-			// If this is our first request, then we allow it.
+		case t.maxSlack == 0 && now-timeOfNextPermissionIssue > int64(t.perRequest):


I still need to grok the code, but briefly looking at this, I wonder if we should do:

if t.maxSlack == 0 { t.maxSlack == t.preRequest }

during the initialization to get rid of one of the cases.

Unfortunately, no, because this will allow for one additional permission to accumulate.
In general, I think we can simplify this and other implementations if we change the way how we initialize rate limiter state, but this is a subject for a separate PR, I think.

Hm. I will need to really think about this more.

In your current proposal though, are we not changing the behavior for the very first request?

I might add tests for the first request only to formalize the behavior.

In your current proposal though, are we not changing the behavior for the very first request?

Yup, I just mentioned a potential future simplification change that is not in the scope of this fix.

I'm less worried about implementation simplification than any unspecified behavior - I'd like to make sure that all implementation behave identically.

Tried to address initial few "Takes" in #97 - WDYT?

I might add one or two more tests before looking at the "time" changes.

I'm less worried about implementation simplification than any unspecified behavior - I'd like to make sure that all implementation behave identically.

This fix functional behaves identically to the previous behavior. It just applies the same behavior to cases when no one has used RL for a long time, so it accumulated many permissions. But this limiter has no slack, so we should not allow permissions to accumulate, so we need to move the newTimeOfNextPermissionIssue to now, as if RL was created right now.

See #95 (comment) From that discussion I wasn't sure whether the proposed the initial startup sequence of the limiter - i.e. whether at startup we always block, or always allow. Since we didn't seem to have that codified (perhaps apart from the `example_test.go`) this PR adds a test to verify this. This is still slightly (2/1000) flaky, but I think that's good enough to add this in - should be valuable anyway.

* Add a test verifying initial startup sequence See #95 (comment) From that discussion I wasn't sure whether the proposed the initial startup sequence of the limiter - i.e. whether at startup we always block, or always allow. Since we didn't seem to have that codified (perhaps apart from the `example_test.go`) this PR adds a test to verify this. This is still slightly (2/1000) flaky, but I think that's good enough to add this in - should be valuable anyway. * channels are great

* Add a test verifying initial startup sequence See uber-go#95 (comment) From that discussion I wasn't sure whether the proposed the initial startup sequence of the limiter - i.e. whether at startup we always block, or always allow. Since we didn't seem to have that codified (perhaps apart from the `example_test.go`) this PR adds a test to verify this. This is still slightly (2/1000) flaky, but I think that's good enough to add this in - should be valuable anyway. * channels are great

storozhukBM · 2022-07-13T22:37:15Z

@rabbbit
New test for initial call failed, but only on 1.18, bad thing is that it doesn't show what exact limiter type didn't pass the test.
Maybe we should add additional test logging for that.

storozhukBM · 2022-07-13T22:41:44Z

@rabbbit
I did the same force-push 😄 already

rabbbit · 2022-07-13T22:44:41Z

limiter_atomic_int64.go

 			newTimeOfNextPermissionIssue = now
-		case now-timeOfNextPermissionIssue > int64(t.maxSlack):
+		case t.maxSlack > 0 && now-timeOfNextPermissionIssue > int64(t.maxSlack):


Suggested change

case t.maxSlack > 0 && now-timeOfNextPermissionIssue > int64(t.maxSlack):

now-timeOfNextPermissionIssue > int64(t.maxSlack):

I think the maxSlack check here is unnecessary - the previous iff should have covered that.

Wait, no, this is wrong - > int64(t.maxSlack): vs > int64(t.perRequest)).

I clearly need to understand this code more.

I think there can be a case where t.maxSlack == 0 is true, but now-timeOfNextPermissionIssue > int64(t.perRequest) is false, in that case we will try to evaluate now-timeOfNextPermissionIssue > int64(t.maxSlack) and it can be true, so we will end up in a wrong branch. So we need t.maxSlack > 0 check here to step into default branch.

rabbbit · 2022-07-13T22:49:25Z

Since this is not actually the "main" limiter yet, I'll merge this in and potentially post-review later on.

* Fix return timestamp discrepancy between regular atomic limiter and int64 based one * Make int64 based atomic limiter default Long story: this was added in #85, but reverted in #91 due to #90. #95 fixed the issue, so we're moving forward with the new implementation.

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'll try reverting #95 on a branch temporarily to see if it would have been caught.

storozhukBM marked this pull request as ready for review July 2, 2022 20:58

storozhukBM mentioned this pull request Jul 2, 2022

atomicInt64Limiter WithoutSlack doesn't block #90

Closed

rabbbit reviewed Jul 5, 2022

View reviewed changes

storozhukBM force-pushed the fix_no_slack_option_for_int64_based_limiter branch from e4cf9ea to 22f95cb Compare July 7, 2022 20:50

rabbbit mentioned this pull request Jul 10, 2022

Add a test verifying initial startup sequence #97

Merged

storozhukBM mentioned this pull request Jul 13, 2022

Fix test approach for detecting issues #93

Open

storozhukBM force-pushed the fix_no_slack_option_for_int64_based_limiter branch from dc66526 to b658cc5 Compare July 13, 2022 22:32

Fix no slack option for int64 based option

ef02693

rabbbit force-pushed the fix_no_slack_option_for_int64_based_limiter branch from b658cc5 to ef02693 Compare July 13, 2022 22:40

rabbbit approved these changes Jul 13, 2022

View reviewed changes

rabbbit merged commit b62b799 into uber-go:main Jul 13, 2022

rabbbit mentioned this pull request Oct 31, 2022

Make int64 based atomic ratelimiter default #101

Merged

rabbbit mentioned this pull request Jul 9, 2023

Update clock dependency #116

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix no slack option for int64 based option #95

Fix no slack option for int64 based option #95

storozhukBM commented Jul 2, 2022

codecov bot commented Jul 2, 2022 •

edited

Loading

storozhukBM commented Jul 2, 2022

rabbbit left a comment •

edited

Loading

rabbbit Jul 5, 2022

storozhukBM Jul 5, 2022

rabbbit Jul 6, 2022 •

edited

Loading

storozhukBM Jul 6, 2022

rabbbit Jul 10, 2022

storozhukBM Jul 10, 2022

storozhukBM commented Jul 13, 2022

storozhukBM commented Jul 13, 2022

rabbbit Jul 13, 2022

rabbbit Jul 13, 2022

storozhukBM Jul 13, 2022

rabbbit commented Jul 13, 2022

	case t.maxSlack > 0 && now-timeOfNextPermissionIssue > int64(t.maxSlack):
	now-timeOfNextPermissionIssue > int64(t.maxSlack):

Fix no slack option for int64 based option #95

Fix no slack option for int64 based option #95

Conversation

storozhukBM commented Jul 2, 2022

codecov bot commented Jul 2, 2022 • edited Loading

Codecov Report

storozhukBM commented Jul 2, 2022

rabbbit left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rabbbit Jul 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

storozhukBM commented Jul 13, 2022

storozhukBM commented Jul 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rabbbit commented Jul 13, 2022

codecov bot commented Jul 2, 2022 •

edited

Loading

rabbbit left a comment •

edited

Loading

rabbbit Jul 6, 2022 •

edited

Loading