Skip to content

[RateLimiter] add Limit::ensureAccepted() which throws RateLimitExceededException if not accepted #38354

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
Oct 2, 2020
Merged

[RateLimiter] add Limit::ensureAccepted() which throws RateLimitExceededException if not accepted #38354

merged 1 commit into from
Oct 2, 2020

Conversation

kbond
Copy link
Member

@kbond kbond commented Sep 30, 2020

Q A
Branch? master
Bug fix? no
New feature? yes
Deprecations? no
Tickets Ref #38241 (comment)
License MIT
Doc PR todo

Example:

try {
    $limit = $limiter->consume()->ensureAccepted();
} catch (RateLimitExceededException $e) {
    $limit = $e->getLimit();
}

@carsonbot carsonbot added Status: Needs Review RFC RFC = Request For Comments (proposals about features that you want to be discussed) Feature labels Sep 30, 2020
@nicolas-grekas nicolas-grekas added this to the next milestone Oct 1, 2020
@nicolas-grekas
Copy link
Member

/cc @wouterj

@wouterj
Copy link
Member

wouterj commented Oct 1, 2020

I've been thinking about this one a bit during the past weeks (this is one of 2 major design choices in the RateLimiter component that doesn't yet feel completely grounded at the perfect solution).

I think there are 2 different use-cases for a generic rate limiter that is provided by the component:
A) Check the rate with the intention to drop the overflowing requests
B) Check the rate with the intention to wait till the request can be processed

I would say that this exception is very useful in cases like (A), but a bit difficult in cases like (B). I've not yet created a PR like this, because we can use the current solutions in both cases.

One interesting thing:
The TokenBucketLimiter already has these 2 cases in its interface - as it can reserve tokens in the future: consume() for (A) and reserve() for (B). Maybe we should reintroduce reserve() in the LimiterInterface and somehow create such reservation feature for the window algorithms (although we can't guarantee that they have a valid hit at the retryAt date).

If we manage to separate these 2 cases for the FixedWindow limiter, we can always throw an exception on consume() and return a state at reserve().

@kbond kbond marked this pull request as ready for review October 1, 2020 23:54
@kbond
Copy link
Member Author

kbond commented Oct 1, 2020

@wouterj and I discussed this PR on slack and came to the conclusion that adding Limit::ensureAccepted() which throws a RateLimitExceededException (if not accepted) is the preferred and simplest solution. This behaviour is completely opt-in.

@kbond kbond changed the title [RFC][RateLimiter] have LimiterInterface::consume() throw RateLimitExceededException [RateLimiter] add Limit::ensureAccepted() which throws RateLimitExceededException if not accepted Oct 1, 2020
@fabpot
Copy link
Member

fabpot commented Oct 2, 2020

Thank you @kbond.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature RFC RFC = Request For Comments (proposals about features that you want to be discussed) Status: Reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants