[release-branch.go1.15] net/http: add connections back that haven't been canceled

Issue #41600 fixed the issue when a second request canceled a connection
while the first request was still in roundTrip.
This uncovered a second issue where a request was being canceled (in
roundtrip) but the connection was put back into the idle pool for a
subsequent request.
The fix is the similar except its now in readLoop instead of roundTrip.
A persistent connection is only added back if it successfully removed
the cancel function; otherwise we know the roundTrip has started
cancelRequest.

Fixes #42935.
Updates #42942.

Change-Id: Ia56add20880ccd0c1ab812d380d8628e45f6f44c
Reviewed-on: https://go-review.googlesource.com/c/go/+/274973
Trust: Dmitri Shuralyov <dmitshur@golang.org>
Trust: Damien Neil <dneil@google.com>
Reviewed-by: Damien Neil <dneil@google.com>
(cherry picked from commit 854a2f8e01a554d8052445563863775406a04b71)
Reviewed-on: https://go-review.googlesource.com/c/go/+/297910
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>

diff --git a/src/net/http/transport.go b/src/net/http/transport.go
index 6fb2ea5663f6a7bec809557d21b94320f36c6e63..6e430b9885fcb9c20e0ec1cdf6bba20052d82d6c 100644 (file)
--- a/src/net/http/transport.go
+++ b/src/net/http/transport.go
@@ -766,7 +766,8 @@ func (t *Transport) CancelRequest(req *Request) {
 }
 
 // Cancel an in-flight request, recording the error value.
-func (t *Transport) cancelRequest(key cancelKey, err error) {
+// Returns whether the request was canceled.
+func (t *Transport) cancelRequest(key cancelKey, err error) bool {
        t.reqMu.Lock()
        cancel := t.reqCanceler[key]
        delete(t.reqCanceler, key)
@@ -774,6 +775,8 @@ func (t *Transport) cancelRequest(key cancelKey, err error) {
        if cancel != nil {
                cancel(err)
        }
+
+       return cancel != nil
 }
 
 //
@@ -2087,18 +2090,17 @@ func (pc *persistConn) readLoop() {
                }
 
                if !hasBody || bodyWritable {
-                       pc.t.setReqCanceler(rc.cancelKey, nil)
+                       replaced := pc.t.replaceReqCanceler(rc.cancelKey, nil)
 
                        // Put the idle conn back into the pool before we send the response
                        // so if they process it quickly and make another request, they'll
                        // get this same conn. But we use the unbuffered channel 'rc'
                        // to guarantee that persistConn.roundTrip got out of its select
                        // potentially waiting for this persistConn to close.
-                       // but after
                        alive = alive &&
                                !pc.sawEOF &&
                                pc.wroteRequest() &&
-                               tryPutIdleConn(trace)
+                               replaced && tryPutIdleConn(trace)
 
                        if bodyWritable {
                                closeErr = errCallerOwnsConn
@@ -2160,12 +2162,12 @@ func (pc *persistConn) readLoop() {
                // reading the response body. (or for cancellation or death)
                select {
                case bodyEOF := <-waitForBodyRead:
-                       pc.t.setReqCanceler(rc.cancelKey, nil) // before pc might return to idle pool
+                       replaced := pc.t.replaceReqCanceler(rc.cancelKey, nil) // before pc might return to idle pool
                        alive = alive &&
                                bodyEOF &&
                                !pc.sawEOF &&
                                pc.wroteRequest() &&
-                               tryPutIdleConn(trace)
+                               replaced && tryPutIdleConn(trace)
                        if bodyEOF {
                                eofc <- struct{}{}
                        }
@@ -2561,6 +2563,7 @@ func (pc *persistConn) roundTrip(req *transportRequest) (resp *Response, err err
        cancelChan := req.Request.Cancel
        ctxDoneChan := req.Context().Done()
        pcClosed := pc.closech
+       canceled := false
        for {
                testHookWaitResLoop()
                select {
@@ -2582,8 +2585,7 @@ func (pc *persistConn) roundTrip(req *transportRequest) (resp *Response, err err
                        }
                case <-pcClosed:
                        pcClosed = nil
-                       // check if we are still using the connection
-                       if pc.t.replaceReqCanceler(req.cancelKey, nil) {
+                       if canceled || pc.t.replaceReqCanceler(req.cancelKey, nil) {
                                if debugRoundTrip {
                                        req.logf("closech recv: %T %#v", pc.closed, pc.closed)
                                }
@@ -2607,10 +2609,10 @@ func (pc *persistConn) roundTrip(req *transportRequest) (resp *Response, err err
                        }
                        return re.res, nil
                case <-cancelChan:
-                       pc.t.cancelRequest(req.cancelKey, errRequestCanceled)
+                       canceled = pc.t.cancelRequest(req.cancelKey, errRequestCanceled)
                        cancelChan = nil
                case <-ctxDoneChan:
-                       pc.t.cancelRequest(req.cancelKey, req.Context().Err())
+                       canceled = pc.t.cancelRequest(req.cancelKey, req.Context().Err())
                        cancelChan = nil
                        ctxDoneChan = nil
                }