Skip to content

rpc: Make HTTP server timeout values configurable #17240

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

Merged
karalabe merged 7 commits into from
Jul 31, 2018
Merged

rpc: Make HTTP server timeout values configurable #17240

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
24f28fd
rpc: Make HTTP server timeout values configurable
ryanschneider Jul 24, 2018
f1d697f
rpc: Remove flags for setting HTTP Timeouts, configuring via .toml is…
ryanschneider Jul 27, 2018
99d1ca7
rpc: Replace separate constants with a single default struct.
ryanschneider Jul 27, 2018
9710bb8
rpc: Update HTTP Server Read and Write Timeouts to 30s.
ryanschneider Jul 27, 2018
96a49d9
rpc: Remove redundant NewDefaultHTTPTimeouts function.
ryanschneider Jul 30, 2018
1307a9c
rpc: document HTTPTimeouts.
ryanschneider Jul 30, 2018
254d265
rpc: sanitize timeout values for library use
karalabe Jul 31, 2018
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
2 changes: 1 addition & 1 deletion cmd/clef/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -415,7 +415,7 @@ func signer(c *cli.Context) error {

// start http server
httpEndpoint := fmt.Sprintf("%s:%d", c.String(utils.RPCListenAddrFlag.Name), c.Int(rpcPortFlag.Name))
listener, _, err := rpc.StartHTTPEndpoint(httpEndpoint, rpcAPI, []string{"account"}, cors, vhosts)
listener, _, err := rpc.StartHTTPEndpoint(httpEndpoint, rpcAPI, []string{"account"}, cors, vhosts, rpc.DefaultHTTPTimeouts)
if err != nil {
utils.Fatalf("Could not start RPC api: %v", err)
}
Expand Down
2 changes: 1 addition & 1 deletion node/api.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,7 @@ func (api *PrivateAdminAPI) StartRPC(host *string, port *int, cors *string, apis
}
}

if err := api.node.startHTTP(fmt.Sprintf("%s:%d", *host, *port), api.node.rpcAPIs, modules, allowedOrigins, allowedVHosts); err != nil {
if err := api.node.startHTTP(fmt.Sprintf("%s:%d", *host, *port), api.node.rpcAPIs, modules, allowedOrigins, allowedVHosts, api.node.config.HTTPTimeouts); err != nil {
return false, err
}
return true, nil
Expand Down
5 changes: 5 additions & 0 deletions node/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import (
"github.com/ethereum/go-ethereum/log"
"github.com/ethereum/go-ethereum/p2p"
"github.com/ethereum/go-ethereum/p2p/discover"
"github.com/ethereum/go-ethereum/rpc"
)

const (
Expand Down Expand Up @@ -119,6 +120,10 @@ type Config struct {
// exposed.
HTTPModules []string `toml:",omitempty"`

// HTTPTimeouts allows for customization of the timeout values used by the HTTP RPC
// interface.
HTTPTimeouts rpc.HTTPTimeouts

// WSHost is the host interface on which to start the websocket RPC server. If
// this field is empty, no websocket API endpoint will be started.
WSHost string `toml:",omitempty"`
Expand Down
2 changes: 2 additions & 0 deletions node/defaults.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (

"github.com/ethereum/go-ethereum/p2p"
"github.com/ethereum/go-ethereum/p2p/nat"
"github.com/ethereum/go-ethereum/rpc"
)

const (
Expand All @@ -39,6 +40,7 @@ var DefaultConfig = Config{
HTTPPort: DefaultHTTPPort,
HTTPModules: []string{"net", "web3"},
HTTPVirtualHosts: []string{"localhost"},
HTTPTimeouts: rpc.DefaultHTTPTimeouts,
WSPort: DefaultWSPort,
WSModules: []string{"net", "web3"},
P2P: p2p.Config{
Expand Down
6 changes: 3 additions & 3 deletions node/node.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,7 +263,7 @@ func (n *Node) startRPC(services map[reflect.Type]Service) error {
n.stopInProc()
return err
}
if err := n.startHTTP(n.httpEndpoint, apis, n.config.HTTPModules, n.config.HTTPCors, n.config.HTTPVirtualHosts); err != nil {
if err := n.startHTTP(n.httpEndpoint, apis, n.config.HTTPModules, n.config.HTTPCors, n.config.HTTPVirtualHosts, n.config.HTTPTimeouts); err != nil {
n.stopIPC()
n.stopInProc()
return err
Expand Down Expand Up @@ -331,12 +331,12 @@ func (n *Node) stopIPC() {
}

// startHTTP initializes and starts the HTTP RPC endpoint.
func (n *Node) startHTTP(endpoint string, apis []rpc.API, modules []string, cors []string, vhosts []string) error {
func (n *Node) startHTTP(endpoint string, apis []rpc.API, modules []string, cors []string, vhosts []string, timeouts rpc.HTTPTimeouts) error {
// Short circuit if the HTTP endpoint isn't being exposed
if endpoint == "" {
return nil
}
listener, handler, err := rpc.StartHTTPEndpoint(endpoint, apis, modules, cors, vhosts)
listener, handler, err := rpc.StartHTTPEndpoint(endpoint, apis, modules, cors, vhosts, timeouts)
if err != nil {
return err
}
Expand Down
4 changes: 2 additions & 2 deletions rpc/endpoints.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import (
)

// StartHTTPEndpoint starts the HTTP RPC endpoint, configured with cors/vhosts/modules
func StartHTTPEndpoint(endpoint string, apis []API, modules []string, cors []string, vhosts []string) (net.Listener, *Server, error) {
func StartHTTPEndpoint(endpoint string, apis []API, modules []string, cors []string, vhosts []string, timeouts HTTPTimeouts) (net.Listener, *Server, error) {
// Generate the whitelist based on the allowed modules
whitelist := make(map[string]bool)
for _, module := range modules {
Expand All @@ -47,7 +47,7 @@ func StartHTTPEndpoint(endpoint string, apis []API, modules []string, cors []str
if listener, err = net.Listen("tcp", endpoint); err != nil {
return nil, nil, err
}
go NewHTTPServer(cors, vhosts, handler).Serve(listener)
go NewHTTPServer(cors, vhosts, timeouts, handler).Serve(listener)
return listener, handler, err
}

Expand Down
56 changes: 52 additions & 4 deletions rpc/http.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import (
"sync"
"time"

"github.com/ethereum/go-ethereum/log"
"github.com/rs/cors"
)

Expand Down Expand Up @@ -66,6 +67,38 @@ func (hc *httpConn) Close() error {
return nil
}

// HTTPTimeouts represents the configuration params for the HTTP RPC server.
type HTTPTimeouts struct {
// 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

// 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

// 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, ReadHeaderTimeout is used.
IdleTimeout time.Duration
}

// DefaultHTTPTimeouts represents the default timeout values used if further
// configuration is not provided.
var DefaultHTTPTimeouts = HTTPTimeouts{
ReadTimeout: 30 * time.Second,
WriteTimeout: 30 * time.Second,
IdleTimeout: 120 * time.Second,
}

// DialHTTPWithClient creates a new RPC client that connects to an RPC server over HTTP
// using the provided HTTP Client.
func DialHTTPWithClient(endpoint string, client *http.Client) (*Client, error) {
Expand Down Expand Up @@ -161,15 +194,30 @@ func (t *httpReadWriteNopCloser) Close() error {
// NewHTTPServer creates a new HTTP RPC server around an API provider.
//
// Deprecated: Server implements http.Handler
func NewHTTPServer(cors []string, vhosts []string, srv *Server) *http.Server {
func NewHTTPServer(cors []string, vhosts []string, timeouts HTTPTimeouts, srv *Server) *http.Server {
// Wrap the CORS-handler within a host-handler
handler := newCorsHandler(srv, cors)
handler = newVHostHandler(vhosts, handler)

Copy link
Copy Markdown
Member

@karalabe karalabe Jul 31, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

It would be nice to sanitize the timeout values here. If you use Geth directly, then the code works ok because the defaults are set via CLI flags. If you use go-ethereum as a library however, then this PR is a "breaking" change in its current form because anyone using it needs to specify a new Config field, otherwise the HTTP RPC is bricked due to 0 timeouts.

An elegant solution would be to iterate over the 3 timeout fields here before constructing the http.Server and if any of them is < 1 second (to avoid misused timeouts), reset it to the default:

	if timeouts.ReadTimeout < time.Second {
		log.Warn("Sanitizing invalid HTTP read timeout", "provided", timeouts.ReadTimeout, "updated", DefaultHTTPTimeouts.ReadTimeout)
		timeouts.ReadTimeout = DefaultHTTPTimeouts.ReadTimeout
	}
	if timeouts.WriteTimeout < time.Second {
		log.Warn("Sanitizing invalid HTTP write timeout", "provided", timeouts.WriteTimeout, "updated", DefaultHTTPTimeouts.WriteTimeout)
		timeouts.WriteTimeout = DefaultHTTPTimeouts.WriteTimeout
	}
	if timeouts.IdleTimeout < time.Second {
		log.Warn("Sanitizing invalid HTTP idle timeout", "provided", timeouts.IdleTimeout, "updated", DefaultHTTPTimeouts.IdleTimeout)
		timeouts.IdleTimeout = DefaultHTTPTimeouts.IdleTimeout
	}

Copy link
Copy Markdown
Contributor Author

@ryanschneider ryanschneider Jul 31, 2018

Choose a reason for hiding this comment

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

I just tested this, and actually it appears that the 0 values work just fine, they just disable the timeouts.

Looking at the Go runtime code, it appears that the HTTP server fully expects 0-valued ReadTimeout/WriteTimeout/IdleTimeout fields, since they are the default for Server{}.

For example see this line in http2/server.go, where the ReadTimeout is only used if non-zero:

https://github.com/golang/net/blob/master/http2/server.go#L1818

So actually, when used as a library, go-ethereum 1.8.13 now behaves like it did pre-18.11, before #16880 was merged, where the server ran without any timeouts whatsoever.

I'm not sure if this is desired behavior or not though, but IMO we should allow 0 as a value in the .toml config, as some users may have a good reason to run without timeouts. For example, the Go runtime does behave slightly differently between a 0 timeout and a very long one, wrt context deadlines and such, so someone might have good reason to truly have a 0 timeout when integrating go-ethereum as a library.

Copy link
Copy Markdown
Contributor Author

@ryanschneider ryanschneider Jul 31, 2018

Choose a reason for hiding this comment

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

@karalabe oh, when I made the above comment I hadn't realized you'd added the sanitization (I was tested the un-sanitized initial version just to be clear). I'm not sure I agree with needing the sanitization.

// Make sure timeout values are meaningful
if timeouts.ReadTimeout < time.Second {
log.Warn("Sanitizing invalid HTTP read timeout", "provided", timeouts.ReadTimeout, "updated", DefaultHTTPTimeouts.ReadTimeout)
timeouts.ReadTimeout = DefaultHTTPTimeouts.ReadTimeout
}
if timeouts.WriteTimeout < time.Second {
log.Warn("Sanitizing invalid HTTP write timeout", "provided", timeouts.WriteTimeout, "updated", DefaultHTTPTimeouts.WriteTimeout)
timeouts.WriteTimeout = DefaultHTTPTimeouts.WriteTimeout
}
if timeouts.IdleTimeout < time.Second {
log.Warn("Sanitizing invalid HTTP idle timeout", "provided", timeouts.IdleTimeout, "updated", DefaultHTTPTimeouts.IdleTimeout)
timeouts.IdleTimeout = DefaultHTTPTimeouts.IdleTimeout
}
// Bundle and start the HTTP server
return &http.Server{
Handler: handler,
ReadTimeout: 5 * time.Second,
WriteTimeout: 10 * time.Second,
IdleTimeout: 120 * time.Second,
ReadTimeout: timeouts.ReadTimeout,
WriteTimeout: timeouts.WriteTimeout,
IdleTimeout: timeouts.IdleTimeout,
}
}

Expand Down