net/http: propagate Client.Timeout down into Request's context deadline

Fixes #31657

Change-Id: I85e9595d3ea30d410f1f4b787925a6879a72bdf2
Reviewed-on: https://go-review.googlesource.com/c/go/+/175857
Reviewed-by: Benny Siegert <bsiegert@gmail.com>
Run-TryBot: Benny Siegert <bsiegert@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>

src/net/http/client.go

@@ -10,6 +10,7 @@
 package http
 
 import (
+	"context"
 	"crypto/tls"
 	"encoding/base64"
 	"errors"
@@ -18,6 +19,7 @@ import (
 	"io/ioutil"
 	"log"
 	"net/url"
+	"reflect"
 	"sort"
 	"strings"
 	"sync"
@@ -273,46 +275,95 @@ func send(ireq *Request, rt RoundTripper, deadline time.Time) (resp *Response, d
 	return resp, nil, nil
 }
 
-// setRequestCancel sets the Cancel field of req, if deadline is
-// non-zero. The RoundTripper's type is used to determine whether the legacy
-// CancelRequest behavior should be used.
+// timeBeforeContextDeadline reports whether the non-zero Time t is
+// before ctx's deadline, if any. If ctx does not have a deadline, it
+// always reports true (the deadline is considered infinite).
+func timeBeforeContextDeadline(t time.Time, ctx context.Context) bool {
+	d, ok := ctx.Deadline()
+	if !ok {
+		return true
+	}
+	return t.Before(d)
+}
+
+// knownRoundTripperImpl reports whether rt is a RoundTripper that's
+// maintained by the Go team and known to implement the latest
+// optional semantics (notably contexts).
+func knownRoundTripperImpl(rt RoundTripper) bool {
+	switch rt.(type) {
+	case *Transport, *http2Transport:
+		return true
+	}
+	// There's a very minor chance of a false positive with this.
+	// Insted of detecting our golang.org/x/net/http2.Transport,
+	// it might detect a Transport type in a different http2
+	// package. But I know of none, and the only problem would be
+	// some temporarily leaked goroutines if the transport didn't
+	// support contexts. So this is a good enough heuristic:
+	if reflect.TypeOf(rt).String() == "*http2.Transport" {
+		return true
+	}
+	return false
+}
+
+// setRequestCancel sets req.Cancel and adds a deadline context to req
+// if deadline is non-zero. The RoundTripper's type is used to
+// determine whether the legacy CancelRequest behavior should be used.
 //
 // As background, there are three ways to cancel a request:
 // First was Transport.CancelRequest. (deprecated)
-// Second was Request.Cancel (this mechanism).
+// Second was Request.Cancel.
 // Third was Request.Context.
+// This function populates the second and third, and uses the first if it really needs to.
 func setRequestCancel(req *Request, rt RoundTripper, deadline time.Time) (stopTimer func(), didTimeout func() bool) {
 	if deadline.IsZero() {
 		return nop, alwaysFalse
 	}
+	knownTransport := knownRoundTripperImpl(rt)
+	oldCtx := req.Context()
 
+	if req.Cancel == nil && knownTransport {
+		// If they already had a Request.Context that's
+		// expiring sooner, do nothing:
+		if !timeBeforeContextDeadline(deadline, oldCtx) {
+			return nop, alwaysFalse
+		}
+
+		var cancelCtx func()
+		req.ctx, cancelCtx = context.WithDeadline(oldCtx, deadline)
+		return cancelCtx, func() bool { return time.Now().After(deadline) }
+	}
 	initialReqCancel := req.Cancel // the user's original Request.Cancel, if any
 
+	var cancelCtx func()
+	if oldCtx := req.Context(); timeBeforeContextDeadline(deadline, oldCtx) {
+		req.ctx, cancelCtx = context.WithDeadline(oldCtx, deadline)
+	}
+
 	cancel := make(chan struct{})
 	req.Cancel = cancel
 
 	doCancel := func() {
-		// The newer way (the second way in the func comment):
+		// The second way in the func comment above:
 		close(cancel)
-
-		// The legacy compatibility way, used only
-		// for RoundTripper implementations written
-		// before Go 1.5 or Go 1.6.
-		type canceler interface {
-			CancelRequest(*Request)
-		}
-		switch v := rt.(type) {
-		case *Transport, *http2Transport:
-			// Do nothing. The net/http package's transports
-			// support the new Request.Cancel channel
-		case canceler:
+		// The first way, used only for RoundTripper
+		// implementations written before Go 1.5 or Go 1.6.
+		type canceler interface{ CancelRequest(*Request) }
+		if v, ok := rt.(canceler); ok {
 			v.CancelRequest(req)
 		}
 	}
 
 	stopTimerCh := make(chan struct{})
 	var once sync.Once
-	stopTimer = func() { once.Do(func() { close(stopTimerCh) }) }
+	stopTimer = func() {
+		once.Do(func() {
+			close(stopTimerCh)
+			if cancelCtx != nil {
+				cancelCtx()
+			}
+		})
+	}
 
 	timer := time.NewTimer(time.Until(deadline))
 	var timedOut atomicBool
@@ -870,8 +921,7 @@ func (b *cancelTimerBody) Read(p []byte) (n int, err error) {
 	}
 	if b.reqDidTimeout() {
 		err = &httpError{
-			// TODO: early in cycle: s/Client.Timeout exceeded/timeout or context cancellation/
-			err:     err.Error() + " (Client.Timeout exceeded while reading body)",
+			err:     err.Error() + " (Client.Timeout or context cancellation while reading body)",
 			timeout: true,
 		}
 	}

src/net/http/client_test.go

@@ -1274,7 +1274,7 @@ func testClientTimeout(t *testing.T, h2 bool) {
 		} else if !ne.Timeout() {
 			t.Errorf("net.Error.Timeout = false; want true")
 		}
-		if got := ne.Error(); !strings.Contains(got, "Client.Timeout exceeded") {
+		if got := ne.Error(); !strings.Contains(got, "(Client.Timeout") {
 			t.Errorf("error string = %q; missing timeout substring", got)
 		}
 	case <-time.After(failTime):
@@ -1917,3 +1917,22 @@ func TestClientCloseIdleConnections(t *testing.T) {
 		t.Error("not closed")
 	}
 }
+
+func TestClientPropagatesTimeoutToContext(t *testing.T) {
+	errDial := errors.New("not actually dialing")
+	c := &Client{
+		Timeout: 5 * time.Second,
+		Transport: &Transport{
+			DialContext: func(ctx context.Context, netw, addr string) (net.Conn, error) {
+				deadline, ok := ctx.Deadline()
+				if !ok {
+					t.Error("no deadline")
+				} else {
+					t.Logf("deadline in %v", deadline.Sub(time.Now()).Round(time.Second/10))
+				}
+				return nil, errDial
+			},
+		},
+	}
+	c.Get("https://example.tld/")
+}