Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has acontribution guide which I suggest you to read.

In short:

• Always add tests
• Keep backward compatibility (seehttps://symfony.com/bc).
• Bug fixes must be submitted against the lowest maintained branch where they apply (seehttps://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

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

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

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

Well, you're changingSlidingWindowLimiter, soSlidingWindowLimiterTest sounds like a good place to add a test, doesn't it.

@derrabus, done

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

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

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.

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

@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 theLoginThrottlingListener which checks if a token is available in theCheckPassportEvent listener, if there are none available (as indicated bypeek /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 likeLimiterInterface::peek() to get theRateLimit object partly using the logic fromLimiterInterface::reserve() (where we instantiatingRateLimit). We should handle$tokens === 0 inLimeterInterface::consume() andLimeterInterface::reserve() then.

I can try to do it then or update that PR.
(Anyway we have a BC break here, bcs seems that the oldRateLimit->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?

So then, what about to implement smth likeLimiterInterface::peek() to get theRateLimit object partly using the logic fromLimiterInterface::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.: justconsume(0)).

So then, what about to implement smth likeLimiterInterface::peek() to get theRateLimit object partly using the logic fromLimiterInterface::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.: justconsume(0)).

We're anyway have a BC break here (forsliding_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.

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

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 thatRateLimit->getRetryAfter() returnsnow when the last token was consumed forsliding_window andfixed_window.

Have updated the PR.

@wouterj done.

• PR with fix forfixed_window andtoken_bucket[RateLimiter] Fix results on last token consume #53064
• Updated that PR removingfixed_window fix

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

When putting add($this->limiter, $limit, $limit->getRetryAfter()->getTimestamp(), time()); atvendor/symfony/security-http/EventListener/LoginThrottlingListener.php:55:

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

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

Thanks for the PR :)

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.
Still, note that it isdiscouraged 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 (sodo 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.

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

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 huh, just one more approved but closed PR, ok. Last one was heresymfony/symfony-docs#19205
wtf with my PRs?

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:

> git push git@github.com:ERuban/symfony.gitERROR: Permission to ERuban/symfony.git denied to wouterj.fatal: Could not read from remote repository.Please make sure you have the correct access rightsand 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 :)

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

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?

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.

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

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.

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