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

RateLimit: minSpacing violations count towards window limit #973

Closed
ivangreene opened this issue Mar 8, 2024 · 4 comments · Fixed by #975
Closed

RateLimit: minSpacing violations count towards window limit #973

ivangreene opened this issue Mar 8, 2024 · 4 comments · Fixed by #975
Assignees
Milestone

Comments

@ivangreene
Copy link

I have been experimenting with @RateLimit for an app contact form: I send an email to myself when a new contact is submitted, but want to rate limit so people can't spam me. So I am working with a larger window than normal (6 per hour) so this is observable

I have noticed that RateLimit will not always reach the 'value' of invocations in the window, if the minSpacing is violated. So for example, if I have 6 invocations per hour, and minSpacing = 1, minSpacingUnit = SECONDS, and I submit 6 requests in 1 second, only 1 request makes it through. That is expected. But further spaced out requests within the hour are not invoked. So minSpacing violations count towards the overall window limit. This isn't really what I expected, and I think it's probably not the intention (the docs are not clear on this scenario)

@ivangreene
Copy link
Author

Looking at the impl, this is clear:

boolean allowInvocation = currentPermits > 0;
if (allowInvocation && minSpacingInMillis != 0 && now - lastInvocation < minSpacingInMillis) {
allowInvocation = false;
}
currentPermits--;

Even if the outcome is to deny invocation based on the minSpacing, we still decrement currentPermits

@ivangreene
Copy link
Author

And guarded in SmoothWindow:


This should fix it!

@Ladicek
Copy link
Contributor

Ladicek commented Mar 8, 2024

This actually is documented in the javadoc of @RateLimit:

With fixed and rolling time windows, rejected invocations always count towards the limit, so if a caller continuously invokes the guarded method faster than the configuration allows, all invocations are rejected until the caller slows down. With smooth time windows, rejected invocations do not count towards the recent rate of invocations.

Fixed and rolling strategies are fundamentally very different from the smooth strategy. The first two enforce an invocation count in given time, while the third enforces invocation rate (computed from the limit and time window length).

@Ladicek
Copy link
Contributor

Ladicek commented Mar 8, 2024

I'm adding that paragraph to the reference guide for rate limit as well: #975

Also, one option you have is to specify the rate limit (if you want fixed or rolling) not as 6 per hour, but 1 per 10 minutes.

@Ladicek Ladicek self-assigned this Mar 8, 2024
@Ladicek Ladicek added this to the 6.3.0 milestone Mar 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants