8000 [RateLimiter] Add SlidingWindowLimiter::reserve() by Jeroeny · Pull Request #51676 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[RateLimiter] Add SlidingWindowLimiter::reserve() #51676

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
Merged
merged 1 commit into from
Oct 1, 2023
Merged

[RateLimiter] Add SlidingWindowLimiter::reserve() #51676

merged 1 commit into from
Oct 1, 2023

Conversation

Jeroeny
Copy link
Contributor
@Jeroeny Jeroeny commented Sep 18, 2023 •
edited by fabpot
Loading
Q A
Branch? 6.4
Bug fix? yes
New feature? yes
Deprecations? yes
Tickets Fix #40289, Fixes #46875
License MIT

This implements LimiterInterface->reserve() for SlidingWindowLimiter. I'm not sure if the lack of implementation was due to time/scope or if it's actually not possible and my approach is incorrect. But I like to give it a try anyway. Perhaps @wouterj you could elaborate on that?

The calculation does the following:

  1. Calculate tokens to be released within this window. E.g. if 4 were used in the last window, at 50% into the current, 2 are still to be released.
  2. Calculate the time-per-token within the remainder of the window. If the requested tokens will be available before the end of the current window, return the time-per-token * needed-tokens.
  3. Otherwise return time-per-token of the regular interval * needed-tokens(-after the current window).

This wasn't a bugfix, so back to feature PR. I couldn't reopen #47557, So had to create this new PR.

@carsonbot carsonbot added this to the 6.4 milestone Sep 18, 2023
@Jeroeny Jeroeny mentioned this pull request Sep 18, 2023
Closed
nicolas-grekas
nicolas-grekas reviewed Sep 19, 2023
View reviewed changes
@fabpot
Copy link
Member
fabpot commented Oct 1, 2023

Thank you @Jeroeny.

@fabpot fabpot merged commit f315b4e into symfony:6.4 Oct 1, 2023
@fabpot fabpot mentioned this pull request Oct 1, 2023
Closed
This was referenced Oct 21, 2023
Merged
Merged
@ERuban ERuban mentioned this pull request Nov 30, 2023
Closed
@wouterj wouterj mentioned this pull request Dec 28, 2023
Merged
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
@wouterj wouterj mentioned this pull request Dec 29, 2023
Merged
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabpot fabpot fabpot approved these changes

@nicolas-grekas nicolas-grekas Awaiting requested review from nicolas-grekas

Assignees
No one assigned
Labels
Bug Deprecation Feature RateLimiter Status: Reviewed
Projects
None yet
Milestone
6.4
4 participants
@Jeroeny @fabpot @nicolas-grekas @carsonbot
0