net/http: do not discard body content when closing it within request handlers

(*body).Close() internally tries to discard the content of a request
body up to 256 KB. We rely on this behavior to allow connection re-use,
by calling (*body).Close() when our request handler exits.

Unfortunately, this causes an unfortunate side-effect where we would
prematurely try to discard a body content when (*body).Close() is called
from within a request handler.

There should not be a good reason for (*body).Close() to do this when
called from within a request handler. As such, this CL modifies
(*body).Close() to not discard body contents when called from within a
request handler. Note that when a request handler exits, it will still
try to discard the body content for connection re-use.

For #75933

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

1 files changed, 21 insertions, 10 deletions

diff --git a/src/net/http/server.go b/src/net/http/server.go
index 02554d1a20..5d8e576f71 100644
--- a/src/net/http/server.go
+++ b/src/net/http/server.go
@@ -1077,9 +1077,6 @@ func (c *conn) readRequest(ctx context.Context) (w *response, err error) {
 	req.ctx = ctx
 	req.RemoteAddr = c.remoteAddr
 	req.TLS = c.tlsState
-	if body, ok := req.Body.(*body); ok {
-		body.doEarlyClose = true
-	}
 
 	// Adjust the read deadline if necessary.
 	if !hdrDeadline.Equal(wholeReqDeadline) {
@@ -1711,9 +1708,11 @@ func (w *response) finishRequest() {
 
 	w.conn.r.abortPendingRead()
 
-	// Close the body (regardless of w.closeAfterReply) so we can
-	// re-use its bufio.Reader later safely.
-	w.reqBody.Close()
+	// Try to discard the body (regardless of w.closeAfterReply), so we can
+	// potentially reuse it in the same connection.
+	if b, ok := w.reqBody.(*body); ok {
+		b.tryDiscardBody()
+	}
 
 	if w.req.MultipartForm != nil {
 		w.req.MultipartForm.RemoveAll()
@@ -1741,16 +1740,16 @@ func (w *response) shouldReuseConnection() bool {
 		return false
 	}
 
-	if w.closedRequestBodyEarly() {
+	if w.didIncompleteDiscard() {
 		return false
 	}
 
 	return true
 }
 
-func (w *response) closedRequestBodyEarly() bool {
+func (w *response) didIncompleteDiscard() bool {
 	body, ok := w.req.Body.(*body)
-	return ok && body.didEarlyClose()
+	return ok && body.didIncompleteDiscard()
 }
 
 func (w *response) Flush() {
@@ -2106,6 +2105,18 @@ func (c *conn) serve(ctx context.Context) {
 		// But we're not going to implement HTTP pipelining because it
 		// was never deployed in the wild and the answer is HTTP/2.
 		inFlightResponse = w
+		// Ensure that Close() invocations within request handlers do not
+		// discard the body.
+		if b, ok := w.reqBody.(*body); ok {
+			b.mu.Lock()
+			b.inRequestHandler = true
+			b.mu.Unlock()
+			defer func() {
+				b.mu.Lock()
+				b.inRequestHandler = false
+				b.mu.Unlock()
+			}()
+		}
 		serverHandler{c.server}.ServeHTTP(w, w.req)
 		inFlightResponse = nil
 		w.cancelCtx()
@@ -2116,7 +2127,7 @@ func (c *conn) serve(ctx context.Context) {
 		w.finishRequest()
 		c.rwc.SetWriteDeadline(time.Time{})
 		if !w.shouldReuseConnection() {
-			if w.requestBodyLimitHit || w.closedRequestBodyEarly() {
+			if w.requestBodyLimitHit || w.didIncompleteDiscard() {
 				c.closeWriteAndWait()
 			}
 			return