plugin/forward: Increase minimum read timeout to 200ms (#1889)

After several experiments at SoundCloud we found that the current
minimum read timeout of 10ms is too low. A single request against a
slow/unavailable authoritative server can cause all TCP connections to
get closed. We record a 50th percentile forward/proxy latency of <5ms,
and a 99th percentile latency of 60ms. Using a minimum timeout of 200ms
seems to be a fair trade-off between avoiding unnecessary high
connection churn and reacting to upstream failures in a timely manner.

This change also renames hcDuration to hcInterval to reflect its usage,
and removes the duplicated timeout constant to make code comprehension
easier.

plugin/forward/README.md

@@ -83,8 +83,9 @@ Also note the TLS config is "global" for the whole forwarding proxy if you need
 `tls-name` for different upstreams you're out of luck.
 
 On each endpoint, the timeouts of the communication are set by default and automatically tuned depending early results.
-- dialTimeout by default is 30 sec, and can decrease automatically down to 100ms
-- readTimeout by default is 2 sec, and can decrease automatically down to 10ms
+
+* dialTimeout by default is 30 sec, and can decrease automatically down to 100ms
+* readTimeout by default is 2 sec, and can decrease automatically down to 200ms
 
 ## Metrics
 

plugin/forward/connect.go

@@ -97,7 +97,7 @@ func (p *Proxy) Connect(ctx context.Context, state request.Request, forceTCP, me
 		conn.UDPSize = 512
 	}
 
-	conn.SetWriteDeadline(time.Now().Add(timeout))
+	conn.SetWriteDeadline(time.Now().Add(maxTimeout))
 	reqTime := time.Now()
 	if err := conn.WriteMsg(state.Req); err != nil {
 		conn.Close() // not giving it back
@@ -110,7 +110,7 @@ func (p *Proxy) Connect(ctx context.Context, state request.Request, forceTCP, me
 	conn.SetReadDeadline(time.Now().Add(p.readTimeout()))
 	ret, err := conn.ReadMsg()
 	if err != nil {
-		p.updateRtt(timeout)
+		p.updateRtt(maxTimeout)
 		conn.Close() // not giving it back
 		if err == io.EOF && cached {
 			return nil, ErrCachedClosed

plugin/forward/forward.go

@@ -39,7 +39,7 @@ type Forward struct {
 
 // New returns a new Forward.
 func New() *Forward {
-	f := &Forward{maxfails: 2, tlsConfig: new(tls.Config), expire: defaultExpire, p: new(random), from: ".", hcInterval: hcDuration}
+	f := &Forward{maxfails: 2, tlsConfig: new(tls.Config), expire: defaultExpire, p: new(random), from: ".", hcInterval: hcInterval}
 	return f
 }
 

plugin/forward/proxy.go

@@ -34,7 +34,7 @@ func NewProxy(addr string, tlsConfig *tls.Config) *Proxy {
 		fails:     0,
 		probe:     up.New(),
 		transport: newTransport(addr, tlsConfig),
-		avgRtt:    int64(timeout / 2),
+		avgRtt:    int64(maxTimeout / 2),
 	}
 	p.client = dnsClient(tlsConfig)
 	runtime.SetFinalizer(p, (*Proxy).finalizer)
@@ -106,8 +106,7 @@ func (p *Proxy) start(duration time.Duration) {
 }
 
 const (
-	timeout    = 2 * time.Second
 	maxTimeout = 2 * time.Second
-	minTimeout = 10 * time.Millisecond
-	hcDuration = 500 * time.Millisecond
+	minTimeout = 200 * time.Millisecond
+	hcInterval = 500 * time.Millisecond
 )

plugin/forward/proxy_test.go

@@ -27,7 +27,7 @@ func TestProxyClose(t *testing.T) {
 
 	for i := 0; i < 100; i++ {
 		p := NewProxy(s.Addr, nil)
-		p.start(hcDuration)
+		p.start(hcInterval)
 
 		go func() { p.Connect(ctx, state, false, false) }()
 		go func() { p.Connect(ctx, state, true, false) }()