feat!: gateway: fix rate limiting, better stateful handling #12315
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
4612641 to
537352e
Compare
Minor API changes: * gateway.NewRateLimiterHandler and gateway.NewConnectionRateLimiterHandler have been replaced with gateway.NewRateLimitHandler. * The handlers returned by both gateway.NewRateLimitHandler and the primary gateway.Handler return an http.Handler augmented with a Shutdown(ctx) method to be used for graceful cleanup of resources. Fix: * --per-conn-rate-limit was previously applied as a global rate limiter, effectively making it have the same impact as --rate-limit. This change fixes the behaviour such that --per-conn-rate-limit is applied as a API call limiter within a single connection (i.e. a WebSocket connection). The rate is specified as tokens-per-second, where tokens are relative to the expense of the API call being made.
537352e to
0c7e9af
Compare
|
Going to close this in favour of #12327 |
8 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Minor API changes:
Fix:
effectively making it have the same impact as --rate-limit. This change fixes
the behaviour such that --per-conn-rate-limit is applied as a API call
limiter within a single connection (i.e. a WebSocket connection). The rate
is specified as tokens-per-second, where tokens are relative to the expense
of the API call being made.
I wanted to get to #11589 but in the meantime I hit some hurdles that I decided to fix: the first is that it's not clear what these rate limiting properties are supposed to do, and the second is that they don't all seem to do what they should.
Most of this PR is cleanup, some refactoring, some doc extension, tests added. Big changes are listed above, the main fix is the
--per-conn-rate-limitthing and you can see the problem in the diff whereRateLimiterHandlerhas a global rate limiter that it just reuses for each connection, it ends up inNode#limit()where it's applied along with the global--rate-limitlimiter, so it's basically the same thing.Another major change is to replace the
--conn-per-minutelimit mechanism to a standardrate.Limiterinstead of the custom rate limit implementation. There is a slight change of behaviour with this in that I allow a burst, which the original doesn't. It also has a different cleanup mechanism with a separate goroutine dedicated to this task instead of a goroutine per request that decrements and may cleanup if zero.