rpc: Make HTTP server timeout values configurable #17240

ryanschneider · 2018-07-24T21:29:17Z

This is a fix for #17219 and is an alternative to #17088

In my opinion, whatever timeouts are decided on will not work for everyone, as individual use cases for geth vary substantially. So, this exposes the RPC HTTP timeouts as command line/toml options:

  --rpcreadtimeout value   HTTP-RPC server read timeout (default: 5s)
  --rpcwritetimeout value  HTTP-RPC server write timeout (default: 10s)
  --rpcidletimeout value   HTTP-RPC server idle timeout (default: 2m0s)

[Node.HTTPTimeouts]
ReadTimeout = 5000000000
WriteTimeout = 10000000000
IdleTimeout = 120000000000

Which allows the user to configure their node as best seen fit. With this change I think we can make the defaults more or less aggressive, or keep them where they are.

p.s. I considered a larger refactor where all of the HTTP options are pulled into a separate rpc.HTTPConfig struct in the same way that the P2P options are in P2P p2p.Config. However, that would break existing toml configs so I'd consider that a feature for geth 1.9/2.0.

fjl

I think that's a good idea, just a few nitpicks about details ;)

fjl · 2018-07-26T12:13:56Z

rpc/http.go

+	DefaultHTTPIdleTimeout  = 120 * time.Second
+)
+
+func NewDefaultHTTPTimeouts() HTTPTimeouts {


It'd be easier to put the defaults in a global struct variable, e.g. DefaultTimeouts. Then you wouldn't need the constants.

fjl · 2018-07-26T12:14:38Z

cmd/utils/flags.go

+		Name:  "rpcidletimeout",
+		Usage: "HTTP-RPC server idle timeout",
+		Value: rpc.DefaultHTTPIdleTimeout,
+	}


IMHO flags are not needed for this. Very few people will set this option and they can use the config file.

@fjl the majority of configs can be set from command line so config.toml method is confusing a bit. It would be great to be able to configure this using command line flags.

ryanschneider · 2018-07-26T16:00:19Z

Thanks for the feedback, I should be able to get to these changes today or tomorrow.

But, I just ran into a larger issue I need to debug, in that it seems like the values from the config.toml might be read in correctly. That is, given the config:

[Node.HTTPTimeouts]
ReadTimeout = 60000000000
WriteTimeout = 90000000000
IdleTimeout = 120000000000

[Node.P2P]
MaxPeers = 100

So, change the ReadTimeout to 60s, Wirte to 90s, and MaxPeers to 100, if I run:

geth --config /tmp/geth.toml dumpconfig

The dumped config still has the default timeouts (but but has the correct MaxPeers, which is why I added that as a test case):

#..snip..

[Node.P2P]
MaxPeers = 100
#..snip..

[Node.HTTPTimeouts]
ReadTimeout = 5000000000
WriteTimeout = 10000000000
IdleTimeout = 120000000000

So I need to figure out what's going on there before removing the flag options (I also didn't know flags and config could be mixed, but it appears they can be, so I previously assumed all new features had to be exposed as flags as well).

ryanschneider · 2018-07-27T21:34:37Z

@fjl I've pushed two new commits to address both of the items pointed out by you. And I've pushed a change to the actual defaults to 30s as you requested in #17088.

If you'd prefer I squash all this down to a single commit before accepting let me know and I'll happily do so.

ryanschneider · 2018-07-27T21:36:20Z

Forgot to mention that the issue I ran into was because I was resetting the HTTP timeouts to their defaults in setHTTP when I shouldn't have been; that code was removed as part of the removal of the flags and I verified everything works as expected now.

karalabe · 2018-07-30T09:51:00Z

node/config.go

@@ -119,6 +120,8 @@ type Config struct {
 	// exposed.
 	HTTPModules []string `toml:",omitempty"`

+	HTTPTimeouts rpc.HTTPTimeouts


Please properly document this field. All other fields are extensively detailed, let's not ruing the streak ;)

karalabe · 2018-07-30T09:51:28Z

rpc/http.go

+
+func NewDefaultHTTPTimeouts() HTTPTimeouts {
+	return DefaultHTTPTimeouts
+}


There's really no point to have this method here, you can just use rpc.DefaultHTTPTimeouts instead.

karalabe · 2018-07-30T09:51:46Z

rpc/http.go

+	IdleTimeout time.Duration
+}
+
+var DefaultHTTPTimeouts = HTTPTimeouts{


Please add a doc.

ryanschneider · 2018-07-30T18:41:45Z

@karalabe & @fjl I believe all requested changes have been completed. Let me know if you want me to squash to a single commit before accepting the PR, happy to do so. If I squash, the commit message would be:

rpc: allow for configuration of RPC HTTP timeouts, increase default Read/WriteTimeout to 30s.

karalabe · 2018-07-31T09:08:44Z

rpc/http.go

 	// Wrap the CORS-handler within a host-handler
 	handler := newCorsHandler(srv, cors)
 	handler = newVHostHandler(vhosts, handler)
+


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 }

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.

@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.

… sufficient.

) * rpc: Make HTTP server timeout values configurable * rpc: Remove flags for setting HTTP Timeouts, configuring via .toml is sufficient. * rpc: Replace separate constants with a single default struct. * rpc: Update HTTP Server Read and Write Timeouts to 30s. * rpc: Remove redundant NewDefaultHTTPTimeouts function. * rpc: document HTTPTimeouts. * rpc: sanitize timeout values for library use

Crypto2 · 2018-08-28T18:30:22Z

Is this going to be added? I don't see --rpcidletimeout, etc. in the --help output running 1.8.14? The new timeouts are causing a lot of issues for us like others with sends timing out so you have to do a bunch of blockchain checks since you don't know if the TX sent or not.

ryanschneider · 2018-08-28T21:45:49Z

This functionality is in 1.8.13+.

It was decided that the command line arguments were too cluttered, so the values are only configurable via .toml, see for example geth dumpconfig:

[Node.HTTPTimeouts]
ReadTimeout = 30000000000
WriteTimeout = 30000000000
IdleTimeout = 120000000000

In addition to your other command line args, you can start geth with --config /path/to/config.toml and just include the above in your config.toml and adjust as needed.

As with other time.Duration values in the toml file, the units are nanoseconds.

fjl requested changes Jul 26, 2018

View reviewed changes

karalabe requested changes Jul 30, 2018

View reviewed changes

karalabe mentioned this pull request Jul 30, 2018

rpc: bump HTTP server timeouts #17088

Closed

karalabe reviewed Jul 31, 2018

View reviewed changes

ryanschneider and others added 7 commits July 31, 2018 12:14

rpc: Make HTTP server timeout values configurable

24f28fd

rpc: Remove flags for setting HTTP Timeouts, configuring via .toml is…

f1d697f

… sufficient.

rpc: Replace separate constants with a single default struct.

99d1ca7

rpc: Update HTTP Server Read and Write Timeouts to 30s.

9710bb8

rpc: Remove redundant NewDefaultHTTPTimeouts function.

96a49d9

rpc: document HTTPTimeouts.

1307a9c

rpc: sanitize timeout values for library use

254d265

karalabe force-pushed the rpc-http-timeouts branch from a6604b0 to 254d265 Compare July 31, 2018 09:14

karalabe approved these changes Jul 31, 2018

View reviewed changes

karalabe added this to the 1.8.13 milestone Jul 31, 2018

karalabe merged commit 5d7e185 into ethereum:master Jul 31, 2018

svechinsky mentioned this pull request Aug 26, 2018

Unable to get traces for specific transactions on rinkeby #17126

Closed

emile mentioned this pull request Sep 19, 2018

SendRawTransaction() sometimes returns an empty response. #17694

Closed

mikeshultz mentioned this pull request Oct 9, 2018

Demanding RPC request fails #17016

Closed

agomezlyte mentioned this pull request Sep 4, 2020

Empty responses when calling view method Consensys/quorum#1039

Closed

KimKyungup mentioned this pull request Oct 20, 2020

Set timeouts for rpc/ws klaytn/klaytn#721

Closed

9 tasks

jeongkyun-oh mentioned this pull request Jan 12, 2021

Added timeout for http/fasthttp server klaytn/klaytn#848

Merged

9 tasks

gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Nov 4, 2024

rpc: Make HTTP server timeout values configurable (ethereum#17240)

a645451

gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Nov 4, 2024

rpc: Make HTTP server timeout values configurable (ethereum#17240)

4b01177

gzliudan mentioned this pull request Nov 4, 2024

upgrade package node XinFinOrg/XDPoSChain#726

Merged

19 tasks

gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Nov 4, 2024

rpc: Make HTTP server timeout values configurable (ethereum#17240)

97a5ff6

wanwiset25 pushed a commit to XinFinOrg/XDPoSChain that referenced this pull request Nov 13, 2024

rpc: Make HTTP server timeout values configurable (ethereum#17240)

ae13b67

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rpc: Make HTTP server timeout values configurable #17240

rpc: Make HTTP server timeout values configurable #17240

ryanschneider commented Jul 24, 2018

fjl left a comment

fjl Jul 26, 2018

fjl Jul 26, 2018

linuxion Aug 29, 2018

ryanschneider commented Jul 26, 2018

ryanschneider commented Jul 27, 2018

ryanschneider commented Jul 27, 2018

karalabe Jul 30, 2018

karalabe Jul 30, 2018

karalabe Jul 30, 2018

ryanschneider Jul 30, 2018

ryanschneider commented Jul 30, 2018

karalabe Jul 31, 2018 •

edited

Loading

ryanschneider Jul 31, 2018

ryanschneider Jul 31, 2018

Crypto2 commented Aug 28, 2018

ryanschneider commented Aug 28, 2018

rpc: Make HTTP server timeout values configurable #17240

rpc: Make HTTP server timeout values configurable #17240

Conversation

ryanschneider commented Jul 24, 2018

fjl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanschneider commented Jul 26, 2018

ryanschneider commented Jul 27, 2018

ryanschneider commented Jul 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanschneider commented Jul 30, 2018

karalabe Jul 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Crypto2 commented Aug 28, 2018

ryanschneider commented Aug 28, 2018

karalabe Jul 31, 2018 •

edited

Loading