add http rpc timeout option as per (#23416) #23483
Conversation
reds
requested review from
holiman,
karalabe,
rjl493456442 and
zsfelfoldi
as code owners
…ethereum#23416) documentation update
cmd/utils/flags.go
Outdated
| Name: "http.timeout", | ||
| Usage: "Sets http rpc request timeout (use \"s\" suffix for seconds, eg: --http.timeout 10s)", | ||
| // uses time.ParseDuration() which requires a unit suffix, "h", "m", "s", "ms" etc. | ||
| Value: fmt.Sprint(ethconfig.Defaults.HTTPRpcTimeout.Nanoseconds(), "ns"), |
There was a problem hiding this comment.
Don't we have (or time lib) have some nicer default printer? It seems a bit silly to print the default in nano-seconds, nicer to have it in seconds or milliseconds.
There was a problem hiding this comment.
Or even better -- IIRC there's some DurationFlag type, isn't there?
There was a problem hiding this comment.
Yep, just printing it out does a better job. 5s instead of 5...ns.
There is a DurationFlag type, nice.
| @@ -972,7 +972,11 @@ func (e *revertError) ErrorData() interface{} { | |||
| // Note, this function doesn't make and changes in the state/blockchain and is | |||
| // useful to execute and retrieve values. | |||
| func (s *PublicBlockChainAPI) Call(ctx context.Context, args TransactionArgs, blockNrOrHash rpc.BlockNumberOrHash, overrides *StateOverride) (hexutil.Bytes, error) { | |||
| result, err := DoCall(ctx, s.b, args, blockNrOrHash, overrides, 5*time.Second, s.b.RPCGasCap()) | |||
| timeout := s.b.HTTPRpcTimeout() | |||
There was a problem hiding this comment.
Is this the only place where we use this? Seems odd, if we have a 'global' rpctimeout flag, to only ever apply it on the Call method?
There was a problem hiding this comment.
Going through internal/ethapi/api.go it seems to be the only rpc with that hard coded timeout. I'll dig in some more.
There was a problem hiding this comment.
There also this:
Line 325 in 5441a8f
A hard coded 5 minute timeout for filters api, eth_getLogs, eth_getFilterChanges etc. Seems incompatible with the 5 second timeout for eth_call.
There was a problem hiding this comment.
It's not the same kind of timeout -- filters generally live on a ws subscription, not a one-off http request/response pair.
| } | ||
| timeout, err := time.ParseDuration(arg) | ||
| if err != nil { | ||
| log.Warn("Bad http.timeout setting", arg) |
There was a problem hiding this comment.
Our log facilities are a bit peculiar: essentially it's on the form log.Level( <message>, <key-name>, <value>, <key-name>, <value>). So it's always 1 + 2N arguments. In this case, it would be appropriate with e.g. log.Warn("Failed to parse http timeout duration", "err", err)
| arg := ctx.GlobalString(HTTPRpcTimeoutFlag.Name) | ||
| _, err := strconv.Atoi(arg) | ||
| if err == nil { | ||
| arg = arg + "ns" | ||
| } |
There was a problem hiding this comment.
Why parse, it's a Duration type already ?
There was a problem hiding this comment.
Yeah, I cleaned this up and added some tests.
|
Closing in favor of #23645. |
No description provided.