8000 [RateLimiter] Fix `RateLimit->getRetryAfter()` return value when consuming `0` or last tokens. by ERuban · Pull Request #52835 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 7 commits into from
Closed

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

wants to merge 7 commits into from

Conversation

ERuban
Copy link
Contributor
@ERuban ERuban commented Nov 30, 2023 •
edited
Loading
Q A
Branch? 6.4
Bug fix? yes
New feature? no
Deprecations? no
License MIT

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.

UPDATE

Have found that RateLimit->getRetryAfter() returns now when the last token was consumed for sliding_window and fixed_window. So fixed it too.

@carsonbot carsonbot added this to the 6.4 milestone Nov 30, 2023
@carsonbot
Copy link

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:

  • Always add tests
  • Keep backward compatibility (see https://symfony.com/bc).
  • Bug fixes must be submitted against the lowest maintained branch where they apply (see https://symfony.com/releases)
  • Features and deprecations must be submitted against the 7.1 branch.

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!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

@derrabus
Copy link
Member

Thank you for your contribution. Please not that we don't merge any changes without a test that covers it.

@carsonbot carsonbot changed the title [RateLimit] Allow to get RateLimit without consuming again. [RateLimiter] [RateLimit] Allow to get RateLimit without consuming again. Nov 30, 2023
@ERuban
Copy link
Contributor Author
ERuban commented Nov 30, 2023

Hey @derrabus,
I'm not sure how and where to test that case. Could you help me a bit?

@derrabus derrabus changed the title [RateLimiter] [RateLimit] Allow to get RateLimit without consuming again. [RateLimiter] Allow to get RateLimit without consuming again. Nov 30, 2023
@derrabus
Copy link
Member

Hey @derrabus, I'm not sure how and where to test that case. Could you help me a bit?

Well, you're changing SlidingWindowLimiter, 8000 so SlidingWindowLimiterTest sounds like a good place to add a test, doesn't it.

@ERuban ERuban force-pushed the fix_rate_limit_bc_after_51676_pr branch from bfecd20 to a2a89d7 Compare November 30, 2023 18:58
@ERuban
Copy link
Contributor Author
ERuban commented Nov 30, 2023

@derrabus, done

@derrabus
Copy link
Member

@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.

@ERuban
Copy link
Contributor Author
ERuban commented Nov 30, 2023

@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.
Just imagine you need to check available tokens somewhere in another class.

@ERuban
Copy link
Contributor Author
ERuban commented Nov 30, 2023 •
edited
Loading

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.

@RobertMe
Copy link
Contributor
RobertMe commented Nov 30, 2023 •
edited
Loading

@derrabus

@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.

This is used in the framework itself too:

symfony/src/Symfony/Component/HttpFoundation/RateLimiter/AbstractRequestRateLimiter.php

Lines 32 to 46 in 8cd61c5

public function peek(Request $request): RateLimit
{
return $this->doConsume($request, 0);
}
private function doConsume(Request $request, int $tokens): RateLimit
{
$limiters = $this->getLimiters($request);
if (0 === \count($limiters)) {
$limiters = [new NoLimiter()];
}
$minimalRateLimit = null;
foreach ($limiters as $limiter) {
$rateLimit = $limiter->consume($tokens);

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.

@ERuban
Copy link
Contributor Author
ERuban commented Nov 30, 2023 •
edited
Loading

@derrabus

@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.

This is used in the framework itself too: https://github.com/symfony/symfony/blob/8cd61c542a7e242d2f1963449b993cfb75c63bf9/src/Symfony/Component/HttpFoundation/RateLimiter/AbstractRequestRateLimiter.php#L34C1-L46C53 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 LimiterInterface::peek() to get the RateLimit object partly using the logic from LimiterInterface::reserve() (where we instantiating RateLimit). We should handle $tokens === 0 in LimeterInterface::consume() and LimeterInterface::reserve() then.

I can try to do it then or update that PR.
(Anyway we have a BC break here, bcs seems that the old RateLimit->getRetryAfter() returns the time when all tokens will be available (if there is no available tokens), and the new one will return the time when the new one will be available)

what do you think?

@RobertMe
Copy link
Contributor

So then, what about to implement smth like LimiterInterface::peek() to get the RateLimit object partly using the logic from LimiterInterface::reserve()

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.
And personally (but I'm not a Symfony dev, just a users) I don't really like the idea as the implementation of such a method would/could be the same for all implementations (i.e.: just consume(0)).

@ERuban
Copy link
Contributor Author
ERuban commented Nov 30, 2023 •
edited
Loading

So then, what about to implement smth like LimiterInterface::peek() to get the RateLimit object partly using the logic from LimiterInterface::reserve()

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. And personally (but I'm not a Symfony dev, just a users) I don't really like the idea as the implementation of such a method would/could be the same for all implementations (i.e.: just consume(0)).

We're anyway have a BC break here (for sliding_window):

Anyway we have a BC break here, bcs seems that the old RateLimit->getRetryAfter() returns the time when all tokens will be available (if there is no available tokens), and the new one will return the time when the new one will be available

Hm, is it a BC break if we will add some new into the interface w/o changing the existing methods? Not sure.

@ERuban
Copy link
Contributor Author
ERuban commented Dec 5, 2023

@derrabus any updates? can we add @wouterj as a reviewer here?

@derrabus
Copy link
Member
derrabus commented Dec 5, 2023

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.

@ERuban
Copy link
Contributor Author
ERuban commented Dec 6, 2023 •
edited
Loading

Have found that RateLimit->getRetryAfter() returns now when the last token was consumed for sliding_window and fixed_window.

Have updated the PR.

@ERuban ERuban changed the title [RateLimiter] Allow to get RateLimit without consuming again. [RateLimiter] Fix RateLimit->getRetryAfter() return value when consuming 0 or last tokens. Dec 6, 2023
@ERuban
Copy link
Contributor Author
ERuban commented Dec 13, 2023

@wouterj done.

@ERuban ERuban force-pushed the fix_rate_limit_bc_after_51676_pr branch from 4691618 to c15d76d Compare December 17, 2023 13:11
@Kocal
Copy link
Member
Kocal commented Dec 18, 2023 •
edited
Loading

Hi everyone, I've also faced the same issue where $limit->getRetryAfter()->getTimestamp() can returns the same value than time():

When putting a dd($this->limiter, $limit, $limit->getRetryAfter()->getTimestamp(), time()); at vendor/symfony/security-http/EventListener/LoginThrottlingListener.php:55:
image

We can see $limit->getRetryAfter()->getTimestamp() and time() returns the same value.

When applying this PR locally, I have now more real values than before:
image

Thanks for the PR :)

@ERuban

This comment was marked as off-topic.

Sign in to view
@wouterj wouterj mentioned this pull request Dec 28, 2023
Merged
@wouterj
Copy link
Member
wouterj commented Dec 28, 2023

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 c 628C overed 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.
Still, note that it is discouraged to use this peeking behavior unless you know what you're doing (e.g. you know the race conditions) and double check consuming a token afterwards is still accepted (so do not copy the example in the PR description).

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.

nicolas-grekas added a commit that referenced this pull request Dec 29, 2023
@nicolas-grekas
bug #53259 [RateLimit] Test and fix peeking behavior on rate limit po…
64f6ac2 
…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
@nicolas-grekas
Copy link
Member
nicolas-grekas commented Dec 29, 2023 •
edited
Loading

I merged #53259 into 6.3 but NOT into 6.4 because I don't know how to resolve conflicts.
@wouterj @ERuban help wanted to resolve this topic on 6.4 🙏

@wouterj wouterj mentioned this pull request Dec 29, 2023
Merged
@wouterj
Copy link
Member
wouterj commented Dec 29, 2023 •
edited
Loading

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
I always find it a bit of a bummer to have to close a PR, but be aware that I kept your authorship over the commit. This means you still get the credits you deserve for fixing this bug. Thanks for finding, reproducing and fixing it!

@wouterj wouterj closed this Dec 29, 2023
@ERuban
Copy link
Contributor Author
ERuban commented Dec 29, 2023

@wouterj huh, just one more approved but closed PR, ok. Last one was here symfony/symfony-docs#19205
wtf with my PRs?

@wouterj
Copy link
Member
wouterj commented Dec 29, 2023

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.

image

@ERuban
Copy link
Contributor Author
ERuban commented Dec 29, 2023

@wouterj ok, done

@wouterj
Copy link
Member
wouterj commented Dec 29, 2023

I'm sorry, it still doesn't seem to work:

> git push git@github.com:ERuban/symfony.git
ERROR: Permission to ERuban/symfony.git denied to wouterj.
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.

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 :)

@ERuban
Copy link
Contributor Author
ERuban commented Dec 29, 2023

ok, nevermind. just let it work at the end)

@ERuban
Copy link
Contributor Author
ERuban commented Dec 29, 2023

also, git push git@github.com:ERuban/symfony.git looks a bit strange for me.
Maybe I'm wrong, but why you push it into my git, why not to push just to that branch, in that repo?

@ERuban
Copy link
Contributor Author
ERuban commented Dec 30, 2023

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.

@nicolas-grekas
Copy link
Member

@ERuban this is how github works, we cannot work around.
You might be happy to know that what matters most from a historical perspective is the git history, and @wouterj made it so you're still the author of the corresponding commit in #53282

nicolas-grekas added a commit that referenced this pull request Dec 30, 2023
@nicolas-grekas
bug #53282 [RateLimiter] Fix RateLimit->getRetryAfter() return value …
4e10a49 
…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
@ERuban
Copy link
Contributor Author
ERuban commented Jan 25, 2024 •
edited
Loading

@nicolas-grekas #53282 (comment)
meh... I'm not sure that I want to do this work again.

Also there a second PR that waiting for you @wouterj a very long time... #53064

@wouterj
Copy link
Member
wouterj commented Jan 25, 2024 •
edited
Loading

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).
Maybe I'll look at your other PR (which is about the bug you mention in your comment on #53282) some time before next release, but I must be honest that messages like this don't really motivate me to do so.

@ERuban
Copy link
Contributor Author
ERuban commented Jan 25, 2024 •
edited
Loading

@wouterj sorry for that, but it was your initiative here at the end of the comment #52835 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@derrabus derrabus derrabus left review comments

Assignees
No one assigned
Labels
Bug RateLimiter Status: Needs Review
Projects
None yet
Milestone
6.4
Development

Successfully merging this pull request may close these issues.
7 participants
@ERuban @carsonbot @derrabus @RobertMe @wouterj @Kocal @nicolas-grekas
0