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

chore: config timeout and connection pool #150

Merged
merged 2 commits into from
Dec 15, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
38 changes: 37 additions & 1 deletion config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -340,6 +340,30 @@ type RPCConfig struct {
// 1024 - 40 - 10 - 50 = 924 = ~900
MaxOpenConnections int `mapstructure:"max_open_connections"`

// mirrors http.Server#ReadTimeout
// ReadTimeout is the maximum duration for reading the entire
// request, including the body.
//
// Because ReadTimeout does not let Handlers make per-request
// decisions on each request body's acceptable deadline or
// upload rate, most users will prefer to use
// ReadHeaderTimeout. It is valid to use them both.
ReadTimeout time.Duration `mapstructure:"read_timeout"`

// mirrors http.Server#WriteTimeout
// WriteTimeout is the maximum duration before timing out
// writes of the response. It is reset whenever a new
// request's header is read. Like ReadTimeout, it does not
// let Handlers make decisions on a per-request basis.
WriteTimeout time.Duration `mapstructure:"write_timeout"`

// mirrors http.Server#IdleTimeout
// IdleTimeout is the maximum amount of time to wait for the
// next request when keep-alives are enabled. If IdleTimeout
// is zero, the value of ReadTimeout is used. If both are
// zero, there is no timeout.
IdleTimeout time.Duration `mapstructure:"idle_timeout"`

// Maximum number of unique clientIDs that can /subscribe
// If you're using /broadcast_tx_commit, set to the estimated maximum number
// of broadcast_tx_commit calls per block.
Expand All @@ -351,7 +375,7 @@ type RPCConfig struct {
MaxSubscriptionsPerClient int `mapstructure:"max_subscriptions_per_client"`

// How long to wait for a tx to be committed during /broadcast_tx_commit
// WARNING: Using a value larger than 10s will result in increasing the
// WARNING: Using a value larger than 'WriteTimeout' will result in increasing the
// global HTTP write timeout, which applies to all connections and endpoints.
// See https://github.com/tendermint/tendermint/issues/3435
TimeoutBroadcastTxCommit time.Duration `mapstructure:"timeout_broadcast_tx_commit"`
Expand Down Expand Up @@ -393,6 +417,9 @@ func DefaultRPCConfig() *RPCConfig {

Unsafe: false,
MaxOpenConnections: 900,
ReadTimeout: 10 * time.Second,
WriteTimeout: 10 * time.Second,
IdleTimeout: 60 * time.Second,

MaxSubscriptionClients: 100,
MaxSubscriptionsPerClient: 5,
Expand Down Expand Up @@ -424,6 +451,15 @@ func (cfg *RPCConfig) ValidateBasic() error {
if cfg.MaxOpenConnections < 0 {
return errors.New("max_open_connections can't be negative")
}
if cfg.ReadTimeout < 0 {
return errors.New("read_timeout can't be negative")
}
if cfg.WriteTimeout < 0 {
return errors.New("write_timeout can't be negative")
}
if cfg.IdleTimeout < 0 {
return errors.New("idle_timeout can't be negative")
}
if cfg.MaxSubscriptionClients < 0 {
return errors.New("max_subscription_clients can't be negative")
}
Expand Down
25 changes: 24 additions & 1 deletion config/toml.go
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,29 @@ unsafe = {{ .RPC.Unsafe }}
# 1024 - 40 - 10 - 50 = 924 = ~900
max_open_connections = {{ .RPC.MaxOpenConnections }}

# mirrors http.Server#ReadTimeout
# ReadTimeout is the maximum duration for reading the entire
# request, including the body.
# Because ReadTimeout does not let Handlers make per-request
# decisions on each request body's acceptable deadline or
# upload rate, most users will prefer to use
# ReadHeaderTimeout. It is valid to use them both.
read_timeout = "{{ .RPC.ReadTimeout }}"

# mirrors http.Server#WriteTimeout
# WriteTimeout is the maximum duration before timing out
# writes of the response. It is reset whenever a new
# request's header is read. Like ReadTimeout, it does not
# let Handlers make decisions on a per-request basis.
write_timeout = "{{ .RPC.WriteTimeout }}"

# mirrors http.Server#IdleTimeout
# IdleTimeout is the maximum amount of time to wait for the
# next request when keep-alives are enabled. If IdleTimeout
# is zero, the value of ReadTimeout is used. If both are
# zero, there is no timeout.
idle_timeout = "{{ .RPC.IdleTimeout }}"

# Maximum number of unique clientIDs that can /subscribe
# If you're using /broadcast_tx_commit, set to the estimated maximum number
# of broadcast_tx_commit calls per block.
Expand All @@ -196,7 +219,7 @@ max_subscription_clients = {{ .RPC.MaxSubscriptionClients }}
max_subscriptions_per_client = {{ .RPC.MaxSubscriptionsPerClient }}

# How long to wait for a tx to be committed during /broadcast_tx_commit.
# WARNING: Using a value larger than 10s will result in increasing the
# WARNING: Using a value larger than 'WriteTimeout' will result in increasing the
# global HTTP write timeout, which applies to all connections and endpoints.
# See https://github.com/tendermint/tendermint/issues/3435
timeout_broadcast_tx_commit = "{{ .RPC.TimeoutBroadcastTxCommit }}"
Expand Down
3 changes: 3 additions & 0 deletions node/node.go
Original file line number Diff line number Diff line change
Expand Up @@ -886,6 +886,9 @@ func (n *Node) startRPC() ([]net.Listener, error) {
config.MaxBodyBytes = n.config.RPC.MaxBodyBytes
config.MaxHeaderBytes = n.config.RPC.MaxHeaderBytes
config.MaxOpenConnections = n.config.RPC.MaxOpenConnections
config.ReadTimeout = n.config.RPC.ReadTimeout
config.WriteTimeout = n.config.RPC.WriteTimeout
config.IdleTimeout = n.config.RPC.IdleTimeout
// If necessary adjust global WriteTimeout to ensure it's greater than
// TimeoutBroadcastTxCommit.
// See https://github.com/tendermint/tendermint/issues/3435
Expand Down
13 changes: 11 additions & 2 deletions rpc/jsonrpc/client/http_json_client.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"net/url"
"strings"
"sync"
"time"

"github.com/pkg/errors"
amino "github.com/tendermint/go-amino"
Expand All @@ -23,6 +24,10 @@ const (
protoWSS = "wss"
protoWS = "ws"
protoTCP = "tcp"

defaultMaxIdleConns = 10000
defaultIdleConnTimeout = 60 // sec
defaultExpectContinueTimeout = 1 // sec
)

//-------------------------------------------------------------
Expand Down Expand Up @@ -369,8 +374,12 @@ func DefaultHTTPClient(remoteAddr string) (*http.Client, error) {
client := &http.Client{
Transport: &http.Transport{
// Set to true to prevent GZIP-bomb DoS attacks
DisableCompression: true,
Dial: dialFn,
DisableCompression: true,
Dial: dialFn,
MaxIdleConns: defaultMaxIdleConns,
MaxIdleConnsPerHost: defaultMaxIdleConns,
IdleConnTimeout: defaultIdleConnTimeout * time.Second,
ExpectContinueTimeout: defaultExpectContinueTimeout * time.Second,
Copy link
Author

@jinsan-line jinsan-line Dec 14, 2020

Choose a reason for hiding this comment

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

I'd like to hear team's opinion.

I tried to change DefaultHTTPClient() function to receive maxIdleConns and idleConnTimeout. After reading the description of the function carefully, I reverted the change and just set defaults.

DefaultHTTPClient is used to create an http client with some default parameters.

Is which one better between just using defaults or receiving maxIdleConns and idleConnTimeout as arguments. Please share your thoughts if you have.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we may just use defaults here.

Copy link
Member

Choose a reason for hiding this comment

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

I also think it's better to use defaults since this function name is DefaultHTTPClient.

},
}

Expand Down
5 changes: 5 additions & 0 deletions rpc/jsonrpc/server/http_server.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ type Config struct {
ReadTimeout time.Duration
// mirrors http.Server#WriteTimeout
WriteTimeout time.Duration
// mirrors http.Server#IdleTimeout
IdleTimeout time.Duration
// MaxBodyBytes controls the maximum number of bytes the
// server will read parsing the request body.
MaxBodyBytes int64
Expand All @@ -40,6 +42,7 @@ func DefaultConfig() *Config {
MaxOpenConnections: 0, // unlimited
ReadTimeout: 10 * time.Second,
WriteTimeout: 10 * time.Second,
IdleTimeout: 60 * time.Second,
MaxBodyBytes: int64(1000000), // 1MB
MaxHeaderBytes: 1 << 20, // same as the net/http default
}
Expand All @@ -56,6 +59,7 @@ func Serve(listener net.Listener, handler http.Handler, logger log.Logger, confi
Handler: RecoverAndLogHandler(maxBytesHandler{h: handler, n: config.MaxBodyBytes}, logger),
ReadTimeout: config.ReadTimeout,
WriteTimeout: config.WriteTimeout,
IdleTimeout: config.IdleTimeout,
MaxHeaderBytes: config.MaxHeaderBytes,
}
err := s.Serve(listener)
Expand All @@ -81,6 +85,7 @@ func ServeTLS(
Handler: RecoverAndLogHandler(maxBytesHandler{h: handler, n: config.MaxBodyBytes}, logger),
ReadTimeout: config.ReadTimeout,
WriteTimeout: config.WriteTimeout,
IdleTimeout: config.IdleTimeout,
MaxHeaderBytes: config.MaxHeaderBytes,
}
err := s.ServeTLS(listener, certFile, keyFile)
Expand Down