net/http: fix request cancellation race

When a in-flight request is cancelled, (*Transport).cancelRequest is
called. The cancelRequest function looks up and invokes a cancel
function before returning. The function lookup happens with reqMu held,
but the cancel function is invoked after dropping the mutex.

If two calls to cancelRequest are made at the same time, it is possible
for one to return before the cancel function has been invoked.

This race causes flakiness in TestClientTimeoutCancel:
  - The test cancels a request while a read from the request body is
    pending.
  - One goroutine calls (*Transport).cancelRequest. This goroutine
    will eventually invoke the cancel function.
  - Another goroutine calls (*Transport).cancelRequest and closes the
    request body. The cancelRequest call returns without invoking
    the cancel function.
  - The read from the request body returns an error. The reader
    checks to see if the request has been canceled, but concludes
    that it has not (because the cancel function hasn't been invoked
    yet).

To avoid this race condition, call the cancel function with the
transport reqMu mutex held.

Calling the cancel function with the mutex held does not introduce any
deadlocks that I can see. The only non-noop request cancel functions
are:

A send to a buffered channel:
https://go.googlesource.com/go/+/refs/heads/master/src/net/http/transport.go#1362

The (*persistConn).cancelRequest function, which does not cancel any
other requests:
https://go.googlesource.com/go/+/refs/heads/master/src/net/http/transport.go#2526

Fixes #34658.

Change-Id: I1b83dce9b0b1d5cf7c7da7dbd03d0fc90c9f5038
Reviewed-on: https://go-review.googlesource.com/c/go/+/303489
Trust: Damien Neil <dneil@google.com>
Run-TryBot: Damien Neil <dneil@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>

diff --git a/src/net/http/transport.go b/src/net/http/transport.go
index 6358c3897ec4774c20e496b6f71ee3cd60e71942..f30ca881ac21d1f8d201e5ad5e0b8be28cee011c 100644 (file)
--- a/src/net/http/transport.go
+++ b/src/net/http/transport.go
@@ -786,10 +786,12 @@ func (t *Transport) CancelRequest(req *Request) {
 // Cancel an in-flight request, recording the error value.
 // Returns whether the request was canceled.
 func (t *Transport) cancelRequest(key cancelKey, err error) bool {
+       // This function must not return until the cancel func has completed.
+       // See: https://golang.org/issue/34658
        t.reqMu.Lock()
+       defer t.reqMu.Unlock()
        cancel := t.reqCanceler[key]
        delete(t.reqCanceler, key)
-       t.reqMu.Unlock()
        if cancel != nil {
                cancel(err)
        }