Skip to content
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

[RateLimiter] Expose the limiter state #38241

Closed
wouterj opened this issue Sep 19, 2020 · 4 comments · Fixed by #38257
Closed

[RateLimiter] Expose the limiter state #38241

wouterj opened this issue Sep 19, 2020 · 4 comments · Fixed by #38257

Comments

@wouterj
Copy link
Member

wouterj commented Sep 19, 2020

This is something I've considered while working on the original RateLimiter PR, but decided not to do (yet). Yet, ever since the component is merged, I'm wondering if we shouldn't add it.

Currently, LimiterInterface#consume() returns a boolean. This means that, in the case of false, the process (or client) has no idea when it should retry. The token bucket limiter has the benefit of being able to "reserve" tokens in the future, allowing IO blocking ($limiter->reserve()->wait()), the fixed window limiter does not have this benefit.

Use-cases of exposing rate limiting state

  • There is an IETF Internet-draft on rate limiting HTTP headers (RateLimit-Limit, RateLimit-Remaining, RateLimit-Reset). Exposing the state allows implementing these headers in your application.
  • The login throttler at this moment says "[...] please try again later.". It would be much more user friendly to be able to say "please try again in ... minutes".
  • Processes that "poll" the limiter if they are denied, the block time is less arbitrary and can be controlled by the limiter.

We can see that for instance the Laravel rate limiter implementation is exposing the state for these cases (ref).

Reasons not to expose rate limiter state

  • The values should never be trusted. The hitcount can be increased by other clients between fetching it from the limiter and using it to render a response. Also, any wait time returned cannot guarantee that the process will succeed afterwards, maybe other processes already burst the limit before the current process.

  • Exposing the limiter state to clients can result in malicious usage, as also described by the Internet-Draft:

    7.6. Denial of Service

    "RateLimit" header fields may assume unexpected values by chance or purpose. For example, an excessively high "RateLimit-Remaining" value may be:

    • used by a malicious intermediary to trigger a Denial of Service attack or consume client resources boosting its requests;
    • passed by a misconfigured server;

    or an high "RateLimit-Reset" value could inhibit clients to contact the server.

    Clients MUST validate the received values to mitigate those risks.

  • The DX of the limiter will be a bit less great, as the object needs to be transformed to a boolean:

    // before
    if ($limiter->consume()) {
    
    // after
    if ($limiter->consume()->ok()) {
@kbond
Copy link
Member

kbond commented Sep 19, 2020

I believe having access to the state would be beneficial for the reasons stated above. The cons could be documented to allow the user to decide whether or not to use.

(as an aside) From a DX perspective, I'd prefer if consume would throw a RateLimitExceededException (or maybe an option to allow this) that contains the state (when next available). This would make it easier to use the rate limiter in multiple places and just let the exception to bubble up to an exception event listener to create the 429 response.

try {
    $state = $limiter->consume();
} catch (RateLimitExceededException $e) {
    $state = $e->getState();
}

@javiereguiluz
Copy link
Member

I thought that all popular services did what GitHub does, which is full disclosure of "rate limiting" state: see https://developer.github.com/v3/#rate-limiting

Others with more experience consuming APIs could tell us if this is common or the exception. Thanks!

@jvasseur
Copy link
Contributor

I think exposing the rate limit state is the way to go. If you don't, consumers will retry blindly leading to more load for your server while exposing it would allow consumers to only retry at the right time.

@wouterj
Copy link
Member Author

wouterj commented Sep 21, 2020

Alright, let's expose this information then on calling consume(). Then everyone has free choice how to use it.

I think the best would be to create a new data object (e.g. Limit). And use the Rate Limiter Internet-Draft as a guideline:

  • Provide a getRetryAfter(): \DateTime method, which is the start of the next window or the first free time for token bucket at that moment
  • Provide a getRemainingTokens(): int method if the limit has not yet been reached
  • Maybe provide some information about the limiter (e.g. window time, strategy and total number of tokens)
  • For the compound limiter, the limiter that first reaches its limit should be returned. See the Internet-Draft for an example.

Anyone up to volunteer to implement this before the feature freeze at 31 September?

@wouterj wouterj added this to Wouter's (@wouterj) in Help Wanted Sep 21, 2020
@fabpot fabpot closed this as completed Sep 29, 2020
fabpot added a commit that referenced this issue Sep 30, 2020
…method (Valentin, vasilvestre)

This PR was merged into the 5.2-dev branch.

Discussion
----------

[RateLimiter] Add limit object on RateLimiter consume method

| Q             | A
| ------------- | ---
| Branch?       | master (should be merged in 5.2 before 31 September if possible)
| Bug fix?      | no
| New feature?  | yes <!-- please update src/**/CHANGELOG.md files -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tickets       | Fix #38241
| License       | MIT
| Doc PR        | Not yet :/ <!-- https://github.com/symfony/symfony-docs/pull/X -->

Commits
-------

8f62afc [RateLimiter] Return Limit object on Consume method
fabpot added a commit that referenced this issue Oct 2, 2020
… RateLimitExceededException if not accepted (kbond)

This PR was merged into the 5.2-dev branch.

Discussion
----------

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

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

Example:

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

Commits
-------

a7ecd0e [RateLimiter] add Limit::ensureAccepted() and RateLimitExceededException
symfony-splitter pushed a commit to symfony/rate-limiter that referenced this issue Oct 2, 2020
… RateLimitExceededException if not accepted (kbond)

This PR was merged into the 5.2-dev branch.

Discussion
----------

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

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

Example:

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

Commits
-------

a7ecd0ed08 [RateLimiter] add Limit::ensureAccepted() and RateLimitExceededException
@wouterj wouterj removed this from Wouter's (@wouterj) in Help Wanted Oct 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants
@fabpot @javiereguiluz @kbond @wouterj @jvasseur and others