Skip to content

[RateLimiter] CompoundLimiter was accepting requests even when some limiters already consumed all tokens #51666

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 1 commit into from
Nov 10, 2023

Conversation

10n
Copy link
Contributor

@10n 10n commented Sep 15, 2023

Q A
Branch? 6.3
Bug fix? yes
New feature? no
Deprecations? no
Tickets -
License MIT
Doc PR -

CompoundLimiter is accepting requests when the limit was reached previously.

When processing the limiters and the first one consumes exactly all the remaining tokens (remaining=0, accepted=true) and the next one already reached the limit previously (remaining=0, accepted=0) the $minimalRateLimit is considered the first one that will accept the request (even if it's not the most restrictive).

For example:
CompoundLimiter includes 2 limiters:

  • limiter 1 - remaining 2 tokens
  • limiter 2 - remaining 0 tokens

After consuming 2 tokens each each limiter generates to limits:

  • limiter1->consume(2), generates a limit indicating 0 remaining tokens, accepts the request (it was last permitted)
  • limiter2->consume(2), generates a limit indicating 0 remaining tokens, did not accept the request (it did not have 2 tokens to satisfy the request)

Because both of them have at this moment 0 remaining tokens, the minimum limit that is returned will be the limit from the limiter1 . This means that the CompundLimiter will accept the request, even if the limiter2 should be more restrictive.

If we switch the order in the constructor, the request will be denied. The order should not matter.

@10n 10n changed the title [RateLimier] - CompoundLimiter was accepting requests even when some limiters already consumed all tokens [RateLimtier] - CompoundLimiter was accepting requests even when some limiters already consumed all tokens Sep 15, 2023
@10n 10n changed the title [RateLimtier] - CompoundLimiter was accepting requests even when some limiters already consumed all tokens [RateLimiter] - CompoundLimiter was accepting requests even when some limiters already consumed all tokens Sep 15, 2023
@nicolas-grekas nicolas-grekas added this to the 6.3 milestone Sep 25, 2023
@nicolas-grekas nicolas-grekas changed the title [RateLimiter] - CompoundLimiter was accepting requests even when some limiters already consumed all tokens [RateLimiter] CompoundLimiter was accepting requests even when some limiters already consumed all tokens Sep 29, 2023
Copy link
Member

@kbond kbond left a comment

Choose a reason for hiding this comment

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

I've confirmed this bug and that this PR fixes it.

@nicolas-grekas nicolas-grekas force-pushed the 6.3.4-fix-compound-limiter branch from 463e296 to 65ce7f8 Compare November 10, 2023 07:40
@fabpot fabpot merged commit 82b811d into symfony:6.3 Nov 10, 2023
@fabpot fabpot mentioned this pull request Nov 10, 2023
@nicolas-grekas
Copy link
Member

Thank you @10n

This was referenced Nov 10, 2023
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.

5 participants