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>

diff --git a/src/net/http/client.go b/src/net/http/client.go
index 65a9d51cc6b179b944b78690db2e4fb3900476d2..38612f22ef76643bb2739d2023a34e8337f5fc67 100644 (file)
--- a/src/net/http/client.go
+++ b/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,
                }
        }

diff --git a/src/net/http/client_test.go b/src/net/http/client_test.go
index ebcd6c9147b413440da99d4654f231b864ea2df8..37c0390a739d5c1e50068796385a9dc4a89870d6 100644 (file)
--- a/src/net/http/client_test.go
+++ b/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/")
+}