-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
feat!: gateway: fix rate limiting, better stateful handling #12327
Conversation
38a8ea5
to
754a0da
Compare
754a0da
to
85895be
Compare
c1c5891
to
e5d5408
Compare
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.
Two potential blockers:
- DoS attack vector due to unbounded limiters per host.
- Repeated timer instantiation in loop instead of reusing ticker.
Other than that, I left a bunch of comments/suggestions.
Thank you for working on this @rvagg 🍻
@masih thanks for the thorough review. I've gone through all your feedback and marked as resolved the easy bits here but left as unresolved the things that may either need follow-up or have responses to questions. Would you mind having another look please? |
bd4f35d
to
07fed9f
Compare
I've messed something up in rate limiting in my latest round of changes, |
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.
* CLI takes --eth-max-filters-per-conn * gateway.NewNode() now takes the API plus an optional list of Options
3017789
to
e842d6e
Compare
e842d6e
to
fcd6000
Compare
@masih || @aarshkshah1992 would you mind having one last look over this before I merge please? @masih the final commit should be the delta between your review and now and contains mostly things you raised. |
Ref: filecoin-project/go-jsonrpc#118
Ref: #11589
Ref: #11153
API changes and breakage:
gateway.NewNode
now only takes 1 argument (the api) and a list of options to override defaultsgateway.NewRateLimiterHandler
andgateway.NewConnectionRateLimiterHandler
have been replaced withgateway.NewRateLimitHandler
.gateway.NewRateLimitHandler
and the primarygateway.Handler
return anhttp.Handler
augmented with aShutdown(ctx)
method to be used for graceful cleanup of resources.Events.FilterTTL
has been reduced from 24h to 1h. It's a measure of "how long since this thing was last used", and 24h is way too long for that IMO.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 fixesthe behaviour such that
--per-conn-rate-limit
is applied as a API calllimiter 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.
Improve:
--eth-max-filters-per-conn
to override the default (16) and it now applies to the combined total of eth subs and filters, not separately--conn-per-minute
limit mechanism to a standardrate.Limiter
instead 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.