From 7fc2625ef16c9e271ca3016f761157ec082cc45a Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Tue, 7 May 2019 16:25:05 -0700 Subject: [PATCH] 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 Run-TryBot: Benny Siegert TryBot-Result: Gobot Gobot --- src/net/http/client.go | 90 ++++++++++++++++++++++++++++--------- src/net/http/client_test.go | 21 ++++++++- 2 files changed, 90 insertions(+), 21 deletions(-) diff --git a/src/net/http/client.go b/src/net/http/client.go index 65a9d51cc6..38612f22ef 100644 --- 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 ebcd6c9147..37c0390a73 100644 --- 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/") +} -- 2.50.0