Skip to content

[RateLimiter] Fix RateLimit->getRetryAfter() return value when consuming 0 or last tokens #53282

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Dec 30, 2023

Conversation

wouterj
Copy link
Member

@wouterj wouterj commented Dec 29, 2023

Q A
Branch? 6.4
Bug fix? yes
New feature? no
Deprecations? no
Issues -
License MIT

Replaces #52835 Original description:

Have got some BC after updating the project to Symfony 6.4 (after that PR #51676).

Sometimes I need to get RateLimit object without consuming just before consuming a try, in example:

$rateLimit = $limiter->consume(0);
if ($rateLimit->getRemainingTokens() === 0) {
   throw new SomeException($rateLimit->getRetryAfter());
}
...
$limiter->consume(1)
...

and found that in that case $rateLimit->getRetryAfter() always returns now.

So this PR fixes it.

@carsonbot carsonbot added this to the 6.4 milestone Dec 29, 2023
@OskarStark OskarStark changed the title [RateLimiter] Fix RateLimit->getRetryAfter() return value when consuming 0 or last tokens. [RateLimiter] Fix RateLimit->getRetryAfter() return value when consuming 0 or last tokens Dec 29, 2023
@nicolas-grekas nicolas-grekas force-pushed the fix_rate_limit_bc_after_51676_pr branch from 701dc3c to 677b8b7 Compare December 30, 2023 09:55
@nicolas-grekas
Copy link
Member

Thank you @wouterj.

@nicolas-grekas nicolas-grekas merged commit 4e10a49 into symfony:6.4 Dec 30, 2023
@nicolas-grekas
Copy link
Member

And thank you @ERuban

@wouterj wouterj deleted the fix_rate_limit_bc_after_51676_pr branch December 30, 2023 10:13
This was referenced Dec 30, 2023
@ERuban
Copy link
Contributor

ERuban commented Jan 23, 2024

Huh, have found that this PR didn't fix some cases that was fixed by my ones.

  1. if you will try to check getRetryAfter right after using the last token(s) - then it will always return now() but it should be a time when first attempt will be available

  2. When there is no available tokens and 0 is consumed then getRetryAfter will show the time when all tokens will be available...

/cc @wouterj

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants