-
-
Notifications
You must be signed in to change notification settings - Fork 183
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
Fix RateLimitController aggressive timeout issue #716
Conversation
const previous = (state as any).requests[api][origin] ?? 0; | ||
(state as any).requests[api][origin] = previous + 1; | ||
|
||
if (previous === 0) { | ||
setTimeout( | ||
() => this.resetRequestCount(api, origin), | ||
this.rateLimitTimeout, | ||
); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the intended behavior is that the first request initiates a timeout, during which the requesting origin can make no more than rateLimitCount
number of requests, then my late-night brain assesses that this is correct.
Previously, every request would initiate a new timeout, regardless of whether the previous one had expired, and that's definitely not correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's exactly the conclusion that I came to as well, your described behaviour was the intention, but it may been altered throughout the implementation process without a good test to remind me. That should be corrected with this PR. Feel free to wait for tomorrow to do an actual review 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see how the previous behavior wasn't correct and this makes sense to me. Nice catch!
Description
Fix an issue with RateLimitController where we applied the timeout too aggressively
Checklist