From 6f62f852efca58b99fc421fdbb405f2620a46e0f Mon Sep 17 00:00:00 2001 From: Damien Neil Date: Fri, 19 Mar 2021 22:01:10 -0700 Subject: [PATCH] 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 Run-TryBot: Damien Neil TryBot-Result: Go Bot Reviewed-by: Bryan C. Mills --- src/net/http/transport.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/net/http/transport.go b/src/net/http/transport.go index 6358c3897e..f30ca881ac 100644 --- 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) } -- 2.50.0