Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[RateLimiter] Fix retryAfter value on last token consume (SlidingWindow).#54163
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
base:6.4
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| if ($availableTokens === $tokens) { | ||
| $resetDuration = $window->calculateTimeForTokens($this->limit, $window->getHitCount()); | ||
| $retryAfter = $now + $resetDuration; | ||
| } else { | ||
| $retryAfter = $now; | ||
| } | ||
| $reservation = new Reservation($now, new RateLimit($this->getAvailableTokens($window->getHitCount()), \DateTimeImmutable::createFromFormat('U', floor($retryAfter)), true, $this->limit)); |
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.
Hi!
I was wondering if we were following the spirit and formalism ofthe previous lines of code 69 to 72 :
$resetDuration =$window->calculateTimeForTokens($this->limit,$window->getHitCount());$resetTime = \DateTimeImmutable::createFromFormat('U',$availableTokens ?floor($now) :floor($now +$resetDuration));returnnewReservation($now,newRateLimit($availableTokens,$resetTime,true,$this->limit));
it might be possible to propose the following code:
| if ($availableTokens ===$tokens) { | |
| $resetDuration =$window->calculateTimeForTokens($this->limit,$window->getHitCount()); | |
| $retryAfter =$now +$resetDuration; | |
| }else { | |
| $retryAfter =$now; | |
| } | |
| $reservation =newReservation($now,newRateLimit($this->getAvailableTokens($window->getHitCount()), \DateTimeImmutable::createFromFormat('U',floor($retryAfter)),true,$this->limit)); | |
| $resetDuration =$window->calculateTimeForTokens($this->limit,$window->getHitCount()); | |
| $resetTime = \DateTimeImmutable::createFromFormat('U',$availableTokens ===$tokens ?floor($now +$resetDuration) :floor($now)); | |
| $reservation =newReservation($now,newRateLimit($this->getAvailableTokens($window->getHitCount()),$resetTime,true,$this->limit)); |
Do you think it's worth keeping this consistency or do you think it's too much?
(This suggestion passes the tests.)
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.
Not sure that it makes sense, and imo using if-else is more readable than ternary ones
jprivet-dev commentedMar 6, 2024 • 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.
Hello again! I noticed a small detail: the try {...returnnewReservation($now,newRateLimit($availableTokens,$resetTime,true,$this->limit));... }finally {$this->lock?->release(); } In this case, all the errors in the |
Seems it is not related to that PR, so you can suggest some fixes in a separate PR. |
Uh oh!
There was an error while loading.Please reload this page.
Fix retryAfter value when the last tokens are consumed ($availableTokens === $tokens).
/cc@wouterj 🙏