[RateLimiter] Fix sliding_window misbehaving with stale records #40141
Conversation
|
Hey! I see that this is your first PR. That is great! Welcome! Symfony has a contribution guide which I suggest you to read. In short:
Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change. When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor! I am going to sit back now and wait for the reviews. Cheers! Carsonbot |
1073a52 to
16a3f0c
Compare
|
Essentially only the last 2 commits are needed to fix the issue, but getHitCount() returning <0 even when it is invalid is still a bug worth fixing imho (the 1st commit). If wanted, I can remove the 1st commit from this PR. |
f39d2ae to
5703316
Compare
|
Thank you @xesxen. |
Currently the SlidingWindow RateLimiter returns a negative value for getHitCount if the previous SlidingWindow was too long ago. This results in a really high value from
SlidingWindowLimiter::getAvailableTokens()which is higher than the configured limit.This limits the value of percentOfCurrentTimeframe in
SlidingWindow::getHitCount()to 1 so it can't result in a negative hitcount.The 2nd fix fixes the SlidingWindow instance (essentially) not storing hits if the previous instance is way in the past, as the next instance will still be "in the past". This causes RateLimit to behave as if it were disabled until it has caught up again, which could take a long time when it is configured with a small window size.