[RateLimiter] TokenBucket policy fix for adding tokens with a predefined frequency #44766
Conversation
|
Hey! I think @alexandre-daubois has recently worked with this code. Maybe they can help review this? Cheers! Carsonbot |
|
@wouterj Can you have a look at this PR? |
|
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. |
There was a problem hiding this comment.
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 the calculateNextTokenAvailability() method. This is a leftover and completely wrong method, so we can better prevent user-land code from calling it as well.
|
Thank you @relo-san. |
|
@relo-san Can you work on a PR to deprecate |
|
@fabpot Yes, I will do that. |
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.