net/http: fix request canceler leak on connection close

writeLoop goroutine closes persistConn closech in case of request body
write error which in turn finishes readLoop without removing request canceler.

Fixes #61708

Change-Id: Ib7c832a91b49bc7888a35a4fd2bd692236c04f86
GitHub-Last-Rev: b74b9055e87121d4dc5d97a3f3ef1afe545bc92d
GitHub-Pull-Request: golang/go#62305
Reviewed-on: https://go-review.googlesource.com/c/go/+/523296
Reviewed-by: Carlos Amedee <carlos@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Damien Neil <dneil@google.com>

2 files changed, 63 insertions, 0 deletions

diff --git a/src/net/http/transport.go b/src/net/http/transport.go
index cc590f1b37..44d5515705 100644
--- a/src/net/http/transport.go
+++ b/src/net/http/transport.go
@@ -2277,6 +2277,7 @@ func (pc *persistConn) readLoop() {
 			pc.t.cancelRequest(rc.cancelKey, rc.req.Context().Err())
 		case <-pc.closech:
 			alive = false
+			pc.t.setReqCanceler(rc.cancelKey, nil)
 		}
 
 		testHookReadLoopBeforeNextRead()
diff --git a/src/net/http/transport_test.go b/src/net/http/transport_test.go
index e407d1768a..d3f43cfd9a 100644
--- a/src/net/http/transport_test.go
+++ b/src/net/http/transport_test.go
@@ -6969,3 +6969,65 @@ func testProxyAuthHeader(t *testing.T, mode testMode) {
 	}
 	resp.Body.Close()
 }
+
+// Issue 61708
+func TestTransportReqCancelerCleanupOnRequestBodyWriteError(t *testing.T) {
+	ln := newLocalListener(t)
+	addr := ln.Addr().String()
+
+	done := make(chan struct{})
+	go func() {
+		conn, err := ln.Accept()
+		if err != nil {
+			t.Errorf("ln.Accept: %v", err)
+			return
+		}
+		// Start reading request before sending response to avoid
+		// "Unsolicited response received on idle HTTP channel" RoundTrip error.
+		if _, err := io.ReadFull(conn, make([]byte, 1)); err != nil {
+			t.Errorf("conn.Read: %v", err)
+			return
+		}
+		io.WriteString(conn, "HTTP/1.1 200\r\nContent-Length: 3\r\n\r\nfoo")
+		<-done
+		conn.Close()
+	}()
+
+	didRead := make(chan bool)
+	SetReadLoopBeforeNextReadHook(func() { didRead <- true })
+	defer SetReadLoopBeforeNextReadHook(nil)
+
+	tr := &Transport{}
+
+	// Send a request with a body guaranteed to fail on write.
+	req, err := NewRequest("POST", "http://"+addr, io.LimitReader(neverEnding('x'), 1<<30))
+	if err != nil {
+		t.Fatalf("NewRequest: %v", err)
+	}
+
+	resp, err := tr.RoundTrip(req)
+	if err != nil {
+		t.Fatalf("tr.RoundTrip: %v", err)
+	}
+
+	close(done)
+
+	// Before closing response body wait for readLoopDone goroutine
+	// to complete due to closed connection by writeLoop.
+	<-didRead
+
+	resp.Body.Close()
+
+	// Verify no outstanding requests after readLoop/writeLoop
+	// goroutines shut down.
+	waitCondition(t, 10*time.Millisecond, func(d time.Duration) bool {
+		n := tr.NumPendingRequestsForTesting()
+		if n > 0 {
+			if d > 0 {
+				t.Logf("pending requests = %d after %v (want 0)", n, d)
+			}
+			return false
+		}
+		return true
+	})
+}