[RateLimiter] Fix RateLimit->getRetryAfter() return value when consuming 0 or last tokens.
#52835
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 |
|
Thank you for your contribution. Please not that we don't merge any changes without a test that covers it. |
|
Hey @derrabus, |
Well, you're changing |
bfecd20 to
a2a89d7
Compare
|
@derrabus, done |
src/Symfony/Component/RateLimiter/Tests/Policy/SlidingWindowLimiterTest.php
Outdated
Show resolved
Hide resolved
|
@wouterj Can you have a look at this? Consuming a rate limiter without actually consuming anything feels like a hack. Is this a scenario we support? I understand that it simply has somehow worked previously. |
Yeah, otherwise we need some another way to get current RateLimit. |
|
And, yes, using it before I got the wrong time to retry, it was the time when all tokens will be available, not the first one. With that fix I got the time for the next available token now. |
This is used in the framework itself too: This allows checking whether a token can be consumed. In this case it's used in the LoginThrottlingListener which checks if a token is available in the CheckPassportEvent listener, if there are none available (as indicated by peek / consume(0)) the login will be denied. If it's allowed (still consumable) it will only be actually consumed when the login fails. That's at least one use case where "peeking" / "consume-ing without actually consuming" is used.
|
Nice example, thank you. So then, what about to implement smth like I can try to do it then or update that PR. what do you think? |
That would be a BC break (you can't change the interface). And doing it in a backwards compatible manner would probably mean it becomes a feature, not a bugfix, so would end up in 7.1 earliest. |
We're anyway have a BC break here (for
Hm, is it a BC break if we will add some new into the interface w/o changing the existing methods? Not sure. |
|
I only did an initial triage on this PR. I've mentioned @wouterj because I believe he can conduct a better review on this topic than I could. However, it's SymfonyCon this week, so he's probably busy. |
|
Have found that Have updated the PR. |
RateLimit->getRetryAfter() return value when consuming 0 or last tokens.
|
@wouterj done.
|
4691618 to
c15d76d
Compare
|
Hi everyone, I've also faced the same issue where When putting a We can see When applying this PR locally, I have now more real values than before: Thanks for the PR :) |
This comment was marked as off-topic.
This comment was marked as off-topic.
|
Sorry for causing confusion here, I might have tested a bit wrongly. In any case, I've created a PR for 6.3 to make the behavior across all policies consistent and make sure it's covered by a test: #53259 When that is merged, we can rebase this PR and fix the remaining issues caused by the reservation feature in 6.4. As a little side note, please do not ping maintainers if nothing changed. We're all doing this completely voluntary, so there is no guarantee whatsoever that we're responding within a certain time frame. |
…licies (wouterj) This PR was merged into the 6.3 branch. Discussion ---------- [RateLimit] Test and fix peeking behavior on rate limit policies | Q | A | ------------- | --- | Branch? | 6.3 | Bug fix? | yes | New feature? | no | Deprecations? | no | Issues | Ref #52835 | License | MIT Although heavily discouraged for user-land code, we've implemented peeking behavior for rate limiting in 6.2 with #46110. However, I found that our rate limit policies show very inconsistent behavior on this. As we didn't have great test covering peeking return values, we broke BC with #51676 in 6.4. I propose this PR to verify the behavior of the policies and also make it inconsistent. I target 6.3 because we rely on this in the login throttling since 6.2 and this shows buggy error messages ("try again in 0 minute") when not using the default policy for login throttling. > [!NOTE] > When merging this PR, there will be heavy merge conflicts in the SlidingWindowLimiter. You can ignore the changes in this PR for this policy in 6.4. I'll rebase and update #52835 to fix the sliding window limiter in 6.4+ Commits ------- e4a8c33 [RateLimit] Test and fix peeking behavior on rate limit policies
|
Hi! I've rebased the branch of this PR and reintroduced the testcase added by my PR in 6.3. I've also reorganized your code a bit, to reduce the number of if-else statements (I always struggle reading rate limiter code, so I like to keep things as simple as possible even if this comes with a cost of some code duplication). However, GitHub for some reason doesn't allow me to push to this PR (even though the UI states maintainers should be allowed). So I've started a new PR: #53282 |
|
@wouterj huh, just one more approved but closed PR, ok. Last one was here symfony/symfony-docs#19205 |
|
In the sidebar of this PR, is this box checked or not? If it's not checked, can you maybe check it? I can then try to push to your fork and reopen this one.
|
|
@wouterj ok, done |
|
I'm sorry, it still doesn't seem to work: I haven't had this before, so not sure how to help you resolve this in the future. In any case, keeping your commit is the most important bit of information :) |
|
ok, nevermind. just let it work at the end) |
|
also, |
|
It was my last PR to Symfony. (It is a bit weird to be approved but not merged more than one time) Thanks to all, nice work. |
…when consuming 0 or last tokens (wouterj, ERuban) This PR was merged into the 6.4 branch. Discussion ---------- [RateLimiter] Fix RateLimit->getRetryAfter() return value when consuming 0 or last tokens | 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: > ```php > $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. Commits ------- 677b8b7 [RateLimit] Allow to get RateLimit without consuming again 169e383 Reintroduce peek consume test for sliding window policy
|
@nicolas-grekas #53282 (comment) Also there a second PR that waiting for you @wouterj a very long time... #53064 |
Sorry, I can't recall having any agreement with the Symfony organization or community about my response time. Been enjoying new years, some busy work weeks and a winter holiday, knowing that the BC break in 6.4 is fixed (which this PR was about). |
|
@wouterj sorry for that, but it was your initiative here at the end of the comment #52835 (comment) |
Have got some BC after updating the project to Symfony 6.4 (after that PR #51676).
Sometimes I need to get
RateLimitobject without consuming just before consuming a try, in example:and found that in that case
$rateLimit->getRetryAfter()always returnsnow.So this PR fixes it.
UPDATE
Have found that
RateLimit->getRetryAfter()returnsnowwhen the last token was consumed forsliding_windowandfixed_window. So fixed it too.