net/http: prevent blocking when draining response body after it has been closed

Previously, draining the response body after it has been closed causes
Response.Body.Close to block for longer than it otherwise would. In a
worst-case scenario, this means that we are incurring a 50 ms delay for
each HTTP/1 request that we make.

This CL makes sure that a response body is drained asynchronously and
updates relevant documentations to reflect the current behavior.

For #77370

Change-Id: I2486961bc1ea3d43d727d0aabc7a6ca7dfb166ee
Reviewed-on: https://go-review.googlesource.com/c/go/+/741222
Reviewed-by: Damien Neil <dneil@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Nicholas Husin <husin@google.com>

diff --git a/src/net/http/client.go b/src/net/http/client.go
index d6a801073553f759ea0120bc142a6aab8de4e5c3..f5c5d2e449a876c5f686744e61561b3ee8a46743 100644 (file)
--- a/src/net/http/client.go
+++ b/src/net/http/client.go
@@ -565,6 +565,9 @@ func urlErrorOp(method string) string {
 // read to EOF and closed, the [Client]'s underlying [RoundTripper]
 // (typically [Transport]) may not be able to re-use a persistent TCP
 // connection to the server for a subsequent "keep-alive" request.
+// Note, however, that [Transport] will automatically try to read a
+// [Response] Body to EOF asynchronously up to a conservative limit
+// when a Body is closed.
 //
 // The request Body, if non-nil, will be closed by the underlying
 // Transport, even on errors. The Body may be closed asynchronously after

diff --git a/src/net/http/clientconn_test.go b/src/net/http/clientconn_test.go
index 03d47939aab6685f7e554422fb1d9ad2929891af..1027d75fed22806ce18956baeb04604e5a1e4367 100644 (file)
--- a/src/net/http/clientconn_test.go
+++ b/src/net/http/clientconn_test.go
@@ -13,6 +13,7 @@ import (
        "sync/atomic"
        "testing"
        "testing/synctest"
+       "time"
 )
 
 func TestTransportNewClientConnRoundTrip(t *testing.T) { run(t, testTransportNewClientConnRoundTrip) }
@@ -283,6 +284,9 @@ func TestClientConnReserveAndConsume(t *testing.T) {
                                }
 
                                test.consume(t, cc, mode)
+                               if mode == http1Mode || mode == https1Mode {
+                                       time.Sleep(http.MaxPostCloseReadTime)
+                               }
                                synctest.Wait()
 
                                // State hook should be called, either to report the

diff --git a/src/net/http/response.go b/src/net/http/response.go
index 0c3d7f6d854132bc8104e3f4a43830f977f5ff2a..1ff83a1892afe644db200fc1f795496275d574f6 100644 (file)
--- a/src/net/http/response.go
+++ b/src/net/http/response.go
@@ -61,7 +61,10 @@ type Response struct {
        // a zero-length body. It is the caller's responsibility to
        // close Body. The default HTTP client's Transport may not
        // reuse HTTP/1.x "keep-alive" TCP connections if the Body is
-       // not read to completion and closed.
+       // not read to completion and closed; however, manually reading
+       // the body to completion should not be needed in most cases,
+       // as closing the body will also cause the body to be read to
+       // completion asynchronously, up to a conservative limit.
        //
        // The Body is automatically dechunked if the server replied
        // with a "chunked" Transfer-Encoding.

diff --git a/src/net/http/transport.go b/src/net/http/transport.go
index 1356d20e94c61b12b427d641d09f5f9d8e4b3f28..924e7cfcb0913a140a0c444a391ba59b4610fe38 100644 (file)
--- a/src/net/http/transport.go
+++ b/src/net/http/transport.go
@@ -2476,7 +2476,9 @@ func (pc *persistConn) readLoop() {
                // reading the response body. (or for cancellation or death)
                select {
                case bodyEOF := <-waitForBodyRead:
-                       if !bodyEOF && resp.ContentLength <= maxPostCloseReadBytes {
+                       tryDrain := !bodyEOF && resp.ContentLength <= maxPostCloseReadBytes
+                       if tryDrain {
+                               eofc <- struct{}{}
                                bodyEOF = maybeDrainBody(body.body)
                        }
                        alive = alive &&
@@ -2484,7 +2486,7 @@ func (pc *persistConn) readLoop() {
                                !pc.sawEOF &&
                                pc.wroteRequest() &&
                                tryPutIdleConn(rc.treq)
-                       if bodyEOF {
+                       if !tryDrain && bodyEOF {
                                eofc <- struct{}{}
                        }
                case <-rc.treq.ctx.Done():