Dialer timeout #4511
Dialer timeout #4511
Conversation
| @@ -145,6 +148,11 @@ type HTTPThrottle struct { | |||
| ThrottleWindow int `mapstructure:"throttle_window"` | |||
| } | |||
|
|
|||
| type NetDialer struct { | |||
There was a problem hiding this comment.
Why not call this Dialer to match the mapstructure name?
| @@ -145,6 +148,11 @@ type HTTPThrottle struct { | |||
| ThrottleWindow int `mapstructure:"throttle_window"` | |||
| } | |||
|
|
|||
| type NetDialer struct { | |||
| Timeout int `mapstructure:"timeout_seconds"` | |||
| KeepAlive int `mapstructure:"keep_alive_seconds"` | |||
There was a problem hiding this comment.
I recommend including the measurement interval in the code field name too, TimeoutSeconds and KeepAliveSeconds.
| v.SetDefault("price_floors.fetcher.http_client.tls_handshake_timeout_seconds", 10) | ||
| v.SetDefault("price_floors.fetcher.http_client.expect_continue_timeout_seconds", 1) | ||
| v.SetDefault("price_floors.fetcher.http_client.dialer.timeout_seconds", 30) | ||
| v.SetDefault("price_floors.fetcher.http_client.dialer.keep_alive_seconds", 30) |
There was a problem hiding this comment.
Same here. The DefaultTransport has a default of 30. The default value of the Dialer is 15. Which is in use today to avoid a breaking change?
| @@ -1220,6 +1236,10 @@ func SetupViper(v *viper.Viper, filename string, bidderInfos BidderInfos) { | |||
| v.SetDefault("price_floors.fetcher.http_client.max_idle_connections", 40) | |||
| v.SetDefault("price_floors.fetcher.http_client.max_idle_connections_per_host", 2) | |||
| v.SetDefault("price_floors.fetcher.http_client.idle_connection_timeout_seconds", 60) | |||
| v.SetDefault("price_floors.fetcher.http_client.tls_handshake_timeout_seconds", 10) | |||
| v.SetDefault("price_floors.fetcher.http_client.expect_continue_timeout_seconds", 1) | |||
| v.SetDefault("price_floors.fetcher.http_client.dialer.timeout_seconds", 30) | |||
There was a problem hiding this comment.
The DefaultTransport has a default of 30. The default value of the Dialer is 0. Since the Dialer is set to null currently, would the current actual value in use be 0 / infinite? This is not a good default, but this would also be considered a breaking change with potential impact. We need to discuss.
| @@ -292,6 +311,11 @@ func New(cfg *config.Configuration, rateConvertor *currency.RateConverter) (r *R | |||
| return r, nil | |||
| } | |||
|
|
|||
| // Grabbing the same dialer context as the default transport uses | |||
There was a problem hiding this comment.
Comment is misleading. This returns the DialContext of a Dialer as a convenience function. Comment also does not follow Go standards, should start with the function name "defaultTransportDialContext returns..."
The default net/http Transport struct includes some dialer timeout configurations. Our Transports do not include these. This PR adds them, and gives the default Transport values as defaults.