From: Nicholas S. Husin Date: Tue, 18 Nov 2025 17:32:44 +0000 (-0500) Subject: Revert "net/http: do not discard body content when closing it within request handlers" X-Git-Tag: go1.26rc1~246 X-Git-Url: http://www.git.cypherpunks.su/?a=commitdiff_plain;h=2cf9d4b62f;p=gostls13.git Revert "net/http: do not discard body content when closing it within request handlers" This reverts commit cb0d9980f5721715ebb73dd2e580eaa11c2ddee2. Reason for revert: the old behavior seems to be relied on by current users, e.g. https://github.com/connectrpc/connect-go/blob/cb2e11fb88c9a61804043355a619c12d4a30a1a5/protocol_connect.go#L837. For #75933 Change-Id: I996280238e5c70a8d760a0b31e3a13c6a44b8616 Reviewed-on: https://go-review.googlesource.com/c/go/+/721761 LUCI-TryBot-Result: Go LUCI Reviewed-by: Damien Neil Auto-Submit: Nicholas Husin Reviewed-by: Nicholas Husin --- diff --git a/src/net/http/serve_test.go b/src/net/http/serve_test.go index af15593c35..4a16ba02af 100644 --- a/src/net/http/serve_test.go +++ b/src/net/http/serve_test.go @@ -2150,137 +2150,118 @@ func TestServerUnreadRequestBodyLarge(t *testing.T) { } } -type bodyDiscardTest struct { +type handlerBodyCloseTest struct { bodySize int bodyChunked bool reqConnClose bool - shouldDiscardBody bool // should the handler discard body after it exits? + wantEOFSearch bool // should Handler's Body.Close do Reads, looking for EOF? + wantNextReq bool // should it find the next request on the same conn? } -func (t bodyDiscardTest) connectionHeader() string { +func (t handlerBodyCloseTest) connectionHeader() string { if t.reqConnClose { return "Connection: close\r\n" } return "" } -var bodyDiscardTests = [...]bodyDiscardTest{ - // Have: - // - Small body. - // - Content-Length defined. - // Should: - // - Discard remaining body. +var handlerBodyCloseTests = [...]handlerBodyCloseTest{ + // Small enough to slurp past to the next request + + // has Content-Length. 0: { - bodySize: 20 << 10, - bodyChunked: false, - reqConnClose: false, - shouldDiscardBody: true, + bodySize: 20 << 10, + bodyChunked: false, + reqConnClose: false, + wantEOFSearch: true, + wantNextReq: true, }, - // Have: - // - Small body. - // - Chunked (no Content-Length defined). - // Should: - // - Discard remaining body. + // Small enough to slurp past to the next request + + // is chunked. 1: { - bodySize: 20 << 10, - bodyChunked: true, - reqConnClose: false, - shouldDiscardBody: true, + bodySize: 20 << 10, + bodyChunked: true, + reqConnClose: false, + wantEOFSearch: true, + wantNextReq: true, }, - // Have: - // - Small body. - // - Content-Length defined. - // - Connection: close. - // Should: - // - Not discard remaining body (no point as Connection: close). + // Small enough to slurp past to the next request + + // has Content-Length + + // declares Connection: close (so pointless to read more). 2: { - bodySize: 20 << 10, - bodyChunked: false, - reqConnClose: true, - shouldDiscardBody: false, + bodySize: 20 << 10, + bodyChunked: false, + reqConnClose: true, + wantEOFSearch: false, + wantNextReq: false, }, - // Have: - // - Small body. - // - Chunked (no Content-Length defined). - // - Connection: close. - // Should: - // - Discard remaining body (chunked, so it might have trailers). - // - // TODO: maybe skip this if no trailers were declared in the headers. + // Small enough to slurp past to the next request + + // declares Connection: close, + // but chunked, so it might have trailers. + // TODO: maybe skip this search if no trailers were declared + // in the headers. 3: { - bodySize: 20 << 10, - bodyChunked: true, - reqConnClose: true, - shouldDiscardBody: true, + bodySize: 20 << 10, + bodyChunked: true, + reqConnClose: true, + wantEOFSearch: true, + wantNextReq: false, }, - // Have: - // - Large body. - // - Content-Length defined. - // Should: - // - Not discard remaining body (we know it is too large from Content-Length). + // Big with Content-Length, so give up immediately if we know it's too big. 4: { - bodySize: 1 << 20, - bodyChunked: false, - reqConnClose: false, - shouldDiscardBody: false, + bodySize: 1 << 20, + bodyChunked: false, // has a Content-Length + reqConnClose: false, + wantEOFSearch: false, + wantNextReq: false, }, - // Have: - // - Large body. - // - Chunked (no Content-Length defined). - // Should: - // - Discard remaining body (chunked, so we try up to a limit before giving up). + // Big chunked, so read a bit before giving up. 5: { - bodySize: 1 << 20, - bodyChunked: true, - reqConnClose: false, - shouldDiscardBody: true, + bodySize: 1 << 20, + bodyChunked: true, + reqConnClose: false, + wantEOFSearch: true, + wantNextReq: false, }, - // Have: - // - Large body. - // - Content-Length defined. - // - Connection: close. - // Should: - // - Not discard remaining body (Connection: Close, and Content-Length is too large). + // Big with Connection: close, but chunked, so search for trailers. + // TODO: maybe skip this search if no trailers were declared + // in the headers. 6: { - bodySize: 1 << 20, - bodyChunked: false, - reqConnClose: true, - shouldDiscardBody: false, + bodySize: 1 << 20, + bodyChunked: true, + reqConnClose: true, + wantEOFSearch: true, + wantNextReq: false, }, - // Have: - // - Large body. - // - Chunked (no Content-Length defined). - // - Connection: close. - // Should: - // - Discard remaining body (chunked, so it might have trailers). - // - // TODO: maybe skip this if no trailers were declared in the headers. + + // Big with Connection: close, so don't do any reads on Close. + // With Content-Length. 7: { - bodySize: 1 << 20, - bodyChunked: true, - reqConnClose: true, - shouldDiscardBody: true, + bodySize: 1 << 20, + bodyChunked: false, + reqConnClose: true, + wantEOFSearch: false, + wantNextReq: false, }, } -func TestBodyDiscard(t *testing.T) { +func TestHandlerBodyClose(t *testing.T) { setParallel(t) if testing.Short() && testenv.Builder() == "" { t.Skip("skipping in -short mode") } - for i, tt := range bodyDiscardTests { - testBodyDiscard(t, i, tt) + for i, tt := range handlerBodyCloseTests { + testHandlerBodyClose(t, i, tt) } } -func testBodyDiscard(t *testing.T, i int, tt bodyDiscardTest) { +func testHandlerBodyClose(t *testing.T, i int, tt handlerBodyCloseTest) { conn := new(testConn) body := strings.Repeat("x", tt.bodySize) if tt.bodyChunked { @@ -2294,12 +2275,12 @@ func testBodyDiscard(t *testing.T, i int, tt bodyDiscardTest) { cw.Close() conn.readBuf.WriteString("\r\n") } else { - conn.readBuf.Write(fmt.Appendf(nil, + conn.readBuf.Write([]byte(fmt.Sprintf( "POST / HTTP/1.1\r\n"+ "Host: test\r\n"+ tt.connectionHeader()+ "Content-Length: %d\r\n"+ - "\r\n", len(body))) + "\r\n", len(body)))) conn.readBuf.Write([]byte(body)) } if !tt.reqConnClose { @@ -2314,23 +2295,26 @@ func testBodyDiscard(t *testing.T, i int, tt bodyDiscardTest) { } ls := &oneConnListener{conn} - var initialSize, closedSize, exitSize int + var numReqs int + var size0, size1 int go Serve(ls, HandlerFunc(func(rw ResponseWriter, req *Request) { - initialSize = readBufLen() - req.Body.Close() - closedSize = readBufLen() + numReqs++ + if numReqs == 1 { + size0 = readBufLen() + req.Body.Close() + size1 = readBufLen() + } })) <-conn.closec - exitSize = readBufLen() - - if initialSize != closedSize { - t.Errorf("%d. Close() within request handler should be a no-op, but body size went from %d to %d", i, initialSize, closedSize) + if numReqs < 1 || numReqs > 2 { + t.Fatalf("%d. bug in test. unexpected number of requests = %d", i, numReqs) } - if tt.shouldDiscardBody && closedSize <= exitSize { - t.Errorf("%d. want body content to be discarded upon request handler exit, but size went from %d to %d", i, closedSize, exitSize) + didSearch := size0 != size1 + if didSearch != tt.wantEOFSearch { + t.Errorf("%d. did EOF search = %v; want %v (size went from %d to %d)", i, didSearch, !didSearch, size0, size1) } - if !tt.shouldDiscardBody && closedSize != exitSize { - t.Errorf("%d. want body content to not be discarded upon request handler exit, but size went from %d to %d", i, closedSize, exitSize) + if tt.wantNextReq && numReqs != 2 { + t.Errorf("%d. numReq = %d; want 2", i, numReqs) } } diff --git a/src/net/http/server.go b/src/net/http/server.go index 5d8e576f71..02554d1a20 100644 --- a/src/net/http/server.go +++ b/src/net/http/server.go @@ -1077,6 +1077,9 @@ 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) { @@ -1708,11 +1711,9 @@ func (w *response) finishRequest() { w.conn.r.abortPendingRead() - // 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() - } + // Close the body (regardless of w.closeAfterReply) so we can + // re-use its bufio.Reader later safely. + w.reqBody.Close() if w.req.MultipartForm != nil { w.req.MultipartForm.RemoveAll() @@ -1740,16 +1741,16 @@ func (w *response) shouldReuseConnection() bool { return false } - if w.didIncompleteDiscard() { + if w.closedRequestBodyEarly() { return false } return true } -func (w *response) didIncompleteDiscard() bool { +func (w *response) closedRequestBodyEarly() bool { body, ok := w.req.Body.(*body) - return ok && body.didIncompleteDiscard() + return ok && body.didEarlyClose() } func (w *response) Flush() { @@ -2105,18 +2106,6 @@ 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() @@ -2127,7 +2116,7 @@ func (c *conn) serve(ctx context.Context) { w.finishRequest() c.rwc.SetWriteDeadline(time.Time{}) if !w.shouldReuseConnection() { - if w.requestBodyLimitHit || w.didIncompleteDiscard() { + if w.requestBodyLimitHit || w.closedRequestBodyEarly() { c.closeWriteAndWait() } return diff --git a/src/net/http/transfer.go b/src/net/http/transfer.go index 9b972aaa44..675551287f 100644 --- a/src/net/http/transfer.go +++ b/src/net/http/transfer.go @@ -809,17 +809,17 @@ func fixTrailer(header Header, chunked bool) (Header, error) { // Close ensures that the body has been fully read // and then reads the trailer if necessary. type body struct { - src io.Reader - hdr any // non-nil (Response or Request) value means read trailer - r *bufio.Reader // underlying wire-format reader for the trailer - closing bool // is the connection to be closed after reading body? - - mu sync.Mutex // guards following, and calls to Read and Close - sawEOF bool - closed bool - incompleteDiscard bool // if true, we failed to discard the body content completely - inRequestHandler bool // used so Close() calls from within request handlers do not discard body - onHitEOF func() // if non-nil, func to call when EOF is Read + src io.Reader + hdr any // non-nil (Response or Request) value means read trailer + r *bufio.Reader // underlying wire-format reader for the trailer + closing bool // is the connection to be closed after reading body? + doEarlyClose bool // whether Close should stop early + + mu sync.Mutex // guards following, and calls to Read and Close + sawEOF bool + closed bool + earlyClose bool // Close called and we didn't read to the end of src + onHitEOF func() // if non-nil, func to call when EOF is Read } // ErrBodyReadAfterClose is returned when reading a [Request] or [Response] @@ -968,69 +968,51 @@ func (b *body) unreadDataSizeLocked() int64 { return -1 } -// shouldDiscardBodyLocked determines whether a body needs to discard its -// content or not. -// b.mu must be held. -func (b *body) shouldDiscardBodyLocked() bool { - // Already saw EOF. No point in discarding the body. - if b.sawEOF { - return false - } - // No trailer and closing the connection next. No point in discarding the - // body since it will not be reusable. - if b.hdr == nil && b.closing { - return false - } - return true -} - -// tryDiscardBody attempts to discard the body content. If the body cannot be -// discarded completely, b.incompleteDiscard will be set to true. -func (b *body) tryDiscardBody() { - b.mu.Lock() - defer b.mu.Unlock() - if !b.shouldDiscardBodyLocked() { - return - } - // Read up to maxPostHandlerReadBytes bytes of the body, looking for EOF - // (and trailers), so we can re-use this connection. - if lr, ok := b.src.(*io.LimitedReader); ok && lr.N > maxPostHandlerReadBytes { - // There was a declared Content-Length, and we have more bytes - // remaining than our maxPostHandlerReadBytes tolerance. So, give up. - b.incompleteDiscard = true - return - } - // Consume the body, which will also lead to us reading the trailer headers - // after the body, if present. - n, err := io.CopyN(io.Discard, bodyLocked{b}, maxPostHandlerReadBytes) - if err == io.EOF { - err = nil - } - if n == maxPostHandlerReadBytes || err != nil { - b.incompleteDiscard = true - } -} - func (b *body) Close() error { b.mu.Lock() defer b.mu.Unlock() if b.closed { return nil } - b.closed = true - if !b.shouldDiscardBodyLocked() || b.inRequestHandler { - return nil + var err error + switch { + case b.sawEOF: + // Already saw EOF, so no need going to look for it. + case b.hdr == nil && b.closing: + // no trailer and closing the connection next. + // no point in reading to EOF. + case b.doEarlyClose: + // Read up to maxPostHandlerReadBytes bytes of the body, looking + // for EOF (and trailers), so we can re-use this connection. + if lr, ok := b.src.(*io.LimitedReader); ok && lr.N > maxPostHandlerReadBytes { + // There was a declared Content-Length, and we have more bytes remaining + // than our maxPostHandlerReadBytes tolerance. So, give up. + b.earlyClose = true + } else { + var n int64 + // Consume the body, or, which will also lead to us reading + // the trailer headers after the body, if present. + n, err = io.CopyN(io.Discard, bodyLocked{b}, maxPostHandlerReadBytes) + if err == io.EOF { + err = nil + } + if n == maxPostHandlerReadBytes { + b.earlyClose = true + } + } + default: + // Fully consume the body, which will also lead to us reading + // the trailer headers after the body, if present. + _, err = io.Copy(io.Discard, bodyLocked{b}) } - // Fully consume the body, which will also lead to us reading - // the trailer headers after the body, if present. - _, err := io.Copy(io.Discard, bodyLocked{b}) + b.closed = true return err } -func (b *body) didIncompleteDiscard() bool { +func (b *body) didEarlyClose() bool { b.mu.Lock() defer b.mu.Unlock() - return b.incompleteDiscard + return b.earlyClose } // bodyRemains reports whether future Read calls might @@ -1054,6 +1036,9 @@ type bodyLocked struct { } func (bl bodyLocked) Read(p []byte) (n int, err error) { + if bl.b.closed { + return 0, ErrBodyReadAfterClose + } return bl.b.readLocked(p) }