Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[RateLimiter] TokenBucket policy fix for adding tokens with a predefined frequency#44766

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

Merged
fabpot merged 1 commit intosymfony:5.4fromrelo-san:ticket_42627
Oct 10, 2023

Conversation

@relo-san
Copy link
Contributor

@relo-sanrelo-san commentedDec 22, 2021
edited by nicolas-grekas
Loading

QA
Branch?5.4
Bug fix?yes
New feature?no
Deprecations?no
TicketsFix#42627
LicenseMIT
Doc PRsymfony/symfony-docs#...

Fixes bug, explained in issue#42627: when interval between requests smaller than rate interval, new tokens can not be added to bucket, because each request updates timer, and therefore rate interval never be reached.
I replace this with approach where timer updates to time, when should have been last updated, and only when rate interval reached.
I added test case for test adding tokens.
As far as I see, there is no BC and no deprecations.
There is method calculateNextTokenAvailability() on Symfony\Component\RateLimiter\Policy\Rate. It does not work correctly (does not match the name and the description), but as far as I can see, it is not used anywhere, so I left it unchanged. The Rate class simply cannot return this data, because it does not have a timer and does not know when the previous addition was made and, accordingly, what to calculate the next from.

@carsonbot
Copy link

Hey!

I think@alexandre-daubois has recently worked with this code. Maybe they can help review this?

Cheers!

Carsonbot

@fabpot
Copy link
Member

@wouterj Can you have a look at this PR?

@fabpotfabpot requested a review fromwouterjAugust 14, 2022 19:59
@nicolas-grekasnicolas-grekas modified the milestones:5.3,5.4Feb 16, 2023
@nicolas-grekasnicolas-grekas changed the base branch from5.3 to5.4February 16, 2023 08:05
@TCM-dev
Copy link

TCM-dev commentedOct 6, 2023
edited
Loading

Are there any news about this MR being merged ? I just discovered this problem myself and tested the fix proposed, it works flawlessly on my end.

When the consumption rate is low enough, I never get throttled, and when it's too high, once everyt okens are consumed, it waits to get new tokens like it's supposed to.

Copy link
Member

@wouterjwouterj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Sorry for stalling so long, I always need some time to get into the rate limit logic again before I can properly review fixes like this.

I think this is a valid fix indeed, nice job! 👍

In 6.4 or 7.1, we can deprecate thecalculateNextTokenAvailability() method. This is a leftover and completely wrong method, so we can better prevent user-land code from calling it as well.

@fabpot
Copy link
Member

Thank you@relo-san.

@fabpotfabpot merged commitadef494 intosymfony:5.4Oct 10, 2023
@fabpot
Copy link
Member

@relo-san Can you work on a PR to deprecatecalculateNextTokenAvailability in 6.4?

@relo-san
Copy link
ContributorAuthor

@fabpot Yes, I will do that.

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

Reviewers

@wouterjwouterjwouterj approved these changes

+1 more reviewer

@maxheliasmaxheliasmaxhelias approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

5.4

Development

Successfully merging this pull request may close these issues.

7 participants

@relo-san@carsonbot@fabpot@TCM-dev@wouterj@maxhelias@nicolas-grekas

[8]ページ先頭

©2009-2025 Movatter.jp