Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[RateLimiter] TokenBucket policy fix for adding tokens with a predefined frequency#44766
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
carsonbot commentedDec 23, 2021
Hey! I think@alexandre-daubois has recently worked with this code. Maybe they can help review this? Cheers! Carsonbot |
fabpot commentedAug 14, 2022
@wouterj Can you have a look at this PR? |
TCM-dev commentedOct 6, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Are there any news about this MR being merged ? I just discovered this problem myself and tested the fix proposed, it works flawlessly on my end. When the consumption rate is low enough, I never get throttled, and when it's too high, once everyt okens are consumed, it waits to get new tokens like it's supposed to. |
wouterj left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Sorry for stalling so long, I always need some time to get into the rate limit logic again before I can properly review fixes like this.
I think this is a valid fix indeed, nice job! 👍
In 6.4 or 7.1, we can deprecate thecalculateNextTokenAvailability() method. This is a leftover and completely wrong method, so we can better prevent user-land code from calling it as well.
fabpot commentedOct 10, 2023
Thank you@relo-san. |
fabpot commentedOct 10, 2023
@relo-san Can you work on a PR to deprecate |
relo-san commentedOct 10, 2023
@fabpot Yes, I will do that. |
Uh oh!
There was an error while loading.Please reload this page.
Fixes bug, explained in issue#42627: when interval between requests smaller than rate interval, new tokens can not be added to bucket, because each request updates timer, and therefore rate interval never be reached.
I replace this with approach where timer updates to time, when should have been last updated, and only when rate interval reached.
I added test case for test adding tokens.
As far as I see, there is no BC and no deprecations.
There is method calculateNextTokenAvailability() on Symfony\Component\RateLimiter\Policy\Rate. It does not work correctly (does not match the name and the description), but as far as I can see, it is not used anywhere, so I left it unchanged. The Rate class simply cannot return this data, because it does not have a timer and does not know when the previous addition was made and, accordingly, what to calculate the next from.