[RateLimiter] fix incorrect retryAfter of FixedWindow #49070
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I'm uncertain about the Psalm failure. |
|
@wouterj it seems like you're the original developer of the RateLimiter component. So may I ask you to have a look at this? (as it seems to have gone unnoticed in general) |
nicolas-grekas
approved these changes
Jul 7, 2023
c612584 to
5c380e9
Compare
A FixedWindow always ends `intervalInSeconds` after the start time (`timer`). So when calculating the time to consume some tokens the tokens will always be available at `timer + intervalInSeconds`. But currently this is reported incorrectly as `calculateTimeForTokens()` calculates the time based on the desired amount of tokens (and cycles) while it doesn't take into account `maxSize` amount of tokens become available at the windows end. Furthermore calculating the amount of needed cycles is incorrect. This as all tokens become available at once (at the windows end) and you can't consume more tokens than `maxSize` (which is validated at the start of `FixedWindowLimiter::reserve`, in case of `tokens > limit` it throws).
5c380e9 to
2316932
Compare
fabpot
approved these changes
Jul 13, 2023
|
Thank you @RobertMe. |
This was referenced Jul 29, 2023
Merged
Merged
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
A FixedWindow always ends
intervalInSecondsafter the start time (timer). So when calculating the time to consume some tokens the tokens will always be available attimer + intervalInSeconds. But currently this is reported incorrectly ascalculateTimeForTokens()calculates the time based on the desired amount of tokens (and cycles) while it doesn't take into accountmaxSizeamount of tokens become available at the windows end.Furthermore calculating the amount of needed cycles is incorrect. This as all tokens become available at once (at the windows end) and you can't consume more tokens than
maxSize(which is validated at the start ofFixedWindowLimiter::reserve, in case oftokens > limitit throws).Note: I don't think that changing the signature of
calculateTimeForTokensis a deprecation. This as theWindowclass is marked as@internal. So it should only be used by theRateLimitercomponent.