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.

Sign up for GitHub

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

services/horizon/internal: Add limit on number of horizon requests in flight #5244

Merged
merged 2 commits into from
Mar 14, 2024
Merged

services/horizon/internal: Add limit on number of horizon requests in flight #5244

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ccd33cd
Add limit on number of horizon requests in flight
tamirms Mar 13, 2024
04fabef
fix defaultMaxConcurrentRequests
tamirms Mar 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions services/horizon/internal/app.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -533,6 +533,7 @@ func (a *App) init() error {
StaleThreshold: a.config.StaleThreshold,
ConnectionTimeout: a.config.ConnectionTimeout,
ClientQueryTimeout: a.config.ClientQueryTimeout,
MaxConcurrentRequests: a.config.MaxConcurrentRequests,
MaxHTTPRequestSize: a.config.MaxHTTPRequestSize,
NetworkPassphrase: a.config.NetworkPassphrase,
MaxPathLength: a.config.MaxPathLength,
Expand Down
11 changes: 6 additions & 5 deletions services/horizon/internal/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,11 +40,12 @@ type Config struct {
ConnectionTimeout time.Duration
ClientQueryTimeout time.Duration
// MaxHTTPRequestSize is the maximum allowed request payload size
MaxHTTPRequestSize uint
RateQuota *throttled.RateQuota
FriendbotURL *url.URL
LogLevel logrus.Level
LogFile string
MaxHTTPRequestSize uint
RateQuota *throttled.RateQuota
MaxConcurrentRequests uint
FriendbotURL *url.URL
LogLevel logrus.Level
LogFile string

// MaxPathLength is the maximum length of the path returned by `/paths` endpoint.
MaxPathLength uint
Expand Down
14 changes: 12 additions & 2 deletions services/horizon/internal/flags.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,8 +69,9 @@ const (
// StellarTestnet is a constant representing the Stellar test network
StellarTestnet = "testnet"

defaultMaxHTTPRequestSize = uint(200 * 1024)
clientQueryTimeoutNotSet = -1
defaultMaxConcurrentRequests = uint(1000)
defaultMaxHTTPRequestSize = uint(200 * 1024)
clientQueryTimeoutNotSet = -1
)

var (
Expand Down Expand Up @@ -471,6 +472,15 @@ func Flags() (*Config, support.ConfigOptions) {
Usage: "sets the limit on the maximum allowed http request payload size, default is 200kb, to disable the limit check, set to 0, only do so if you acknowledge the implications of accepting unbounded http request payload sizes.",
UsedInCommands: ApiServerCommands,
},
&support.ConfigOption{
Name: "max-concurrent-requests",
ConfigKey: &config.MaxConcurrentRequests,
OptType: types.Uint,
FlagDefault: defaultMaxConcurrentRequests,
Usage: "sets the limit on the maximum number of concurrent http requests, default is 1000, to disable the limit set to 0. " +
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just for non-breaking backwards compatibility, should the default be 0 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the default limit is really high that it should never be observed during normal usage. so, although it is technically a breaking change, I think it would be better to have this default so we can protect against OOMs by default

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like we get ~2k/rps on the whole cluster, so yeah 1k/machine should be way more than plenty

"If Horizon receives a request which would exceed the limit of concurrent http requests, Horizon will respond with a 429 status code.",
UsedInCommands: ApiServerCommands,
},
&support.ConfigOption{
Name: "per-hour-rate-limit",
ConfigKey: &config.RateQuota,
Expand Down
12 changes: 8 additions & 4 deletions services/horizon/internal/httpx/router.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,11 @@
)

type RouterConfig struct {
DBSession db.SessionInterface
PrimaryDBSession db.SessionInterface
TxSubmitter *txsub.System
RateQuota *throttled.RateQuota
DBSession db.SessionInterface
PrimaryDBSession db.SessionInterface
TxSubmitter *txsub.System
RateQuota *throttled.RateQuota
MaxConcurrentRequests uint

BehindCloudflare bool
BehindAWSLoadBalancer bool
Expand Down Expand Up @@ -91,6 +92,9 @@
BehindAWSLoadBalancer: config.BehindAWSLoadBalancer,
}))
r.Use(loggerMiddleware(serverMetrics))
if config.MaxConcurrentRequests > 0 {
r.Use(chimiddleware.Throttle(int(config.MaxConcurrentRequests)))

Check failure on line 96 in services/horizon/internal/httpx/router.go

View workflow job for this annotation

GitHub Actions / golangci 

r.Use undefined (type *Router has no field or method Use) (typecheck)
}
r.Use(timeoutMiddleware(config.ConnectionTimeout))
if config.MaxHTTPRequestSize > 0 {
r.Use(func(handler http.Handler) http.Handler {
Expand Down
Loading