Stowage/go
2
0
Fork 0
mirror of https://github.com/golang/go.git synced 2025-12-08 06:10:04 +00:00
Code Issues Projects Releases Packages Wiki Activity

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

Browse source 
This reverts commit cb0d9980f5.

Reason for revert: the old behavior seems to be relied on by current
users, e.g.
cb2e11fb88/protocol_connect.go (L837).

For #75933

Change-Id: I996280238e5c70a8d760a0b31e3a13c6a44b8616
Reviewed-on: https://go-review.googlesource.com/c/go/+/721761
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Damien Neil <dneil@google.com>
Auto-Submit: Nicholas Husin <nsh@golang.org>
Reviewed-by: Nicholas Husin <husin@google.com>
This commit is contained in:
Nicholas S. Husin 2025-11-18 12:32:44 -05:00 • committed by Gopher Robot
parent 4d0658bb08
commit 2cf9d4b62f
3 changed files with 142 additions and 184 deletions
Download patch file Download diff file Expand all files Collapse all files

188
src/net/http/serve_test.go
View file

@ -2150,137 +2150,118 @@ func TestServerUnreadRequestBodyLarge(t *testing.T) {
} }
} }
type bodyDiscardTest struct { type handlerBodyCloseTest struct {
bodySize int bodySize int
bodyChunked bool bodyChunked bool
reqConnClose 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 { if t.reqConnClose {
return "Connection: close\r\n" return "Connection: close\r\n"
} }
return "" return ""
} }
var bodyDiscardTests = [...]bodyDiscardTest{ var handlerBodyCloseTests = [...]handlerBodyCloseTest{
// Have: // Small enough to slurp past to the next request +
// - Small body. // has Content-Length.
// - Content-Length defined.
// Should:
// - Discard remaining body.
0: { 0: {
bodySize: 20 << 10, bodySize: 20 << 10,
bodyChunked: false, bodyChunked: false,
reqConnClose: false, reqConnClose: false,
shouldDiscardBody: true, wantEOFSearch: true,
wantNextReq: true,
}, },
// Have: // Small enough to slurp past to the next request +
// - Small body. // is chunked.
// - Chunked (no Content-Length defined).
// Should:
// - Discard remaining body.
1: { 1: {
bodySize: 20 << 10, bodySize: 20 << 10,
bodyChunked: true, bodyChunked: true,
reqConnClose: false, reqConnClose: false,
shouldDiscardBody: true, wantEOFSearch: true,
wantNextReq: true,
}, },
// Have: // Small enough to slurp past to the next request +
// - Small body. // has Content-Length +
// - Content-Length defined. // declares Connection: close (so pointless to read more).
// - Connection: close.
// Should:
// - Not discard remaining body (no point as Connection: close).
2: { 2: {
bodySize: 20 << 10, bodySize: 20 << 10,
bodyChunked: false, bodyChunked: false,
reqConnClose: true, reqConnClose: true,
shouldDiscardBody: false, wantEOFSearch: false,
wantNextReq: false,
}, },
// Have: // Small enough to slurp past to the next request +
// - Small body. // declares Connection: close,
// - Chunked (no Content-Length defined). // but chunked, so it might have trailers.
// - Connection: close. // TODO: maybe skip this search if no trailers were declared
// Should: // in the headers.
// - Discard remaining body (chunked, so it might have trailers).
//
// TODO: maybe skip this if no trailers were declared in the headers.
3: { 3: {
bodySize: 20 << 10, bodySize: 20 << 10,
bodyChunked: true, bodyChunked: true,
reqConnClose: true, reqConnClose: true,
shouldDiscardBody: true, wantEOFSearch: true,
wantNextReq: false,
}, },
// Have: // Big with Content-Length, so give up immediately if we know it's too big.
// - Large body.
// - Content-Length defined.
// Should:
// - Not discard remaining body (we know it is too large from Content-Length).
4: { 4: {
bodySize: 1 << 20, bodySize: 1 << 20,
bodyChunked: false, bodyChunked: false, // has a Content-Length
reqConnClose: false, reqConnClose: false,
shouldDiscardBody: false, wantEOFSearch: false,
wantNextReq: false,
}, },
// Have: // Big chunked, so read a bit before giving up.
// - Large body.
// - Chunked (no Content-Length defined).
// Should:
// - Discard remaining body (chunked, so we try up to a limit before giving up).
5: { 5: {
bodySize: 1 << 20, bodySize: 1 << 20,
bodyChunked: true, bodyChunked: true,
reqConnClose: false, reqConnClose: false,
shouldDiscardBody: true, wantEOFSearch: true,
wantNextReq: false,
}, },
// Have: // Big with Connection: close, but chunked, so search for trailers.
// - Large body. // TODO: maybe skip this search if no trailers were declared
// - Content-Length defined. // in the headers.
// - Connection: close.
// Should:
// - Not discard remaining body (Connection: Close, and Content-Length is too large).
6: { 6: {
bodySize: 1 << 20, bodySize: 1 << 20,
bodyChunked: false, bodyChunked: true,
reqConnClose: true, reqConnClose: true,
shouldDiscardBody: false, wantEOFSearch: true,
wantNextReq: false,
}, },
// Have:
// - Large body. // Big with Connection: close, so don't do any reads on Close.
// - Chunked (no Content-Length defined). // With Content-Length.
// - Connection: close.
// Should:
// - Discard remaining body (chunked, so it might have trailers).
//
// TODO: maybe skip this if no trailers were declared in the headers.
7: { 7: {
bodySize: 1 << 20, bodySize: 1 << 20,
bodyChunked: true, bodyChunked: false,
reqConnClose: true, reqConnClose: true,
shouldDiscardBody: true, wantEOFSearch: false,
wantNextReq: false,
}, },
} }
func TestBodyDiscard(t *testing.T) { func TestHandlerBodyClose(t *testing.T) {
setParallel(t) setParallel(t)
if testing.Short() && testenv.Builder() == "" { if testing.Short() && testenv.Builder() == "" {
t.Skip("skipping in -short mode") t.Skip("skipping in -short mode")
} }
for i, tt := range bodyDiscardTests { for i, tt := range handlerBodyCloseTests {
testBodyDiscard(t, i, tt) 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) conn := new(testConn)
body := strings.Repeat("x", tt.bodySize) body := strings.Repeat("x", tt.bodySize)
if tt.bodyChunked { if tt.bodyChunked {
@ -2294,12 +2275,12 @@ func testBodyDiscard(t *testing.T, i int, tt bodyDiscardTest) {
cw.Close() cw.Close()
conn.readBuf.WriteString("\r\n") conn.readBuf.WriteString("\r\n")
} else { } else {
conn.readBuf.Write(fmt.Appendf(nil, conn.readBuf.Write([]byte(fmt.Sprintf(
"POST / HTTP/1.1\r\n"+ "POST / HTTP/1.1\r\n"+
"Host: test\r\n"+ "Host: test\r\n"+
tt.connectionHeader()+ tt.connectionHeader()+
"Content-Length: %d\r\n"+ "Content-Length: %d\r\n"+
"\r\n", len(body))) "\r\n", len(body))))
conn.readBuf.Write([]byte(body)) conn.readBuf.Write([]byte(body))
} }
if !tt.reqConnClose { if !tt.reqConnClose {
@ -2314,23 +2295,26 @@ func testBodyDiscard(t *testing.T, i int, tt bodyDiscardTest) {
} }
ls := &oneConnListener{conn} ls := &oneConnListener{conn}
var initialSize, closedSize, exitSize int var numReqs int
var size0, size1 int
go Serve(ls, HandlerFunc(func(rw ResponseWriter, req *Request) { go Serve(ls, HandlerFunc(func(rw ResponseWriter, req *Request) {
initialSize = readBufLen() numReqs++
req.Body.Close() if numReqs == 1 {
closedSize = readBufLen() size0 = readBufLen()
req.Body.Close()
size1 = readBufLen()
}
})) }))
<-conn.closec <-conn.closec
exitSize = readBufLen() if numReqs < 1 || numReqs > 2 {
t.Fatalf("%d. bug in test. unexpected number of requests = %d", i, numReqs)
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 tt.shouldDiscardBody && closedSize <= exitSize { didSearch := size0 != size1
t.Errorf("%d. want body content to be discarded upon request handler exit, but size went from %d to %d", i, closedSize, exitSize) 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 { if tt.wantNextReq && numReqs != 2 {
t.Errorf("%d. want body content to not be discarded upon request handler exit, but size went from %d to %d", i, closedSize, exitSize) t.Errorf("%d. numReq = %d; want 2", i, numReqs)
} }
} }

31
src/net/http/server.go
View file

@ -1077,6 +1077,9 @@ func (c *conn) readRequest(ctx context.Context) (w *response, err error) {
req.ctx = ctx req.ctx = ctx
req.RemoteAddr = c.remoteAddr req.RemoteAddr = c.remoteAddr
req.TLS = c.tlsState req.TLS = c.tlsState
if body, ok := req.Body.(*body); ok {
body.doEarlyClose = true
}
// Adjust the read deadline if necessary. // Adjust the read deadline if necessary.
if !hdrDeadline.Equal(wholeReqDeadline) { if !hdrDeadline.Equal(wholeReqDeadline) {
@ -1708,11 +1711,9 @@ func (w *response) finishRequest() {
w.conn.r.abortPendingRead() w.conn.r.abortPendingRead()
// Try to discard the body (regardless of w.closeAfterReply), so we can // Close the body (regardless of w.closeAfterReply) so we can
// potentially reuse it in the same connection. // re-use its bufio.Reader later safely.
if b, ok := w.reqBody.(*body); ok { w.reqBody.Close()
b.tryDiscardBody()
}
if w.req.MultipartForm != nil { if w.req.MultipartForm != nil {
w.req.MultipartForm.RemoveAll() w.req.MultipartForm.RemoveAll()
@ -1740,16 +1741,16 @@ func (w *response) shouldReuseConnection() bool {
return false return false
} }
if w.didIncompleteDiscard() { if w.closedRequestBodyEarly() {
return false return false
} }
return true return true
} }
func (w *response) didIncompleteDiscard() bool { func (w *response) closedRequestBodyEarly() bool {
body, ok := w.req.Body.(*body) body, ok := w.req.Body.(*body)
return ok && body.didIncompleteDiscard() return ok && body.didEarlyClose()
} }
func (w *response) Flush() { 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 // But we're not going to implement HTTP pipelining because it
// was never deployed in the wild and the answer is HTTP/2. // was never deployed in the wild and the answer is HTTP/2.
inFlightResponse = w 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) serverHandler{c.server}.ServeHTTP(w, w.req)
inFlightResponse = nil inFlightResponse = nil
w.cancelCtx() w.cancelCtx()
@ -2127,7 +2116,7 @@ func (c *conn) serve(ctx context.Context) {
w.finishRequest() w.finishRequest()
c.rwc.SetWriteDeadline(time.Time{}) c.rwc.SetWriteDeadline(time.Time{})
if !w.shouldReuseConnection() { if !w.shouldReuseConnection() {
if w.requestBodyLimitHit || w.didIncompleteDiscard() { if w.requestBodyLimitHit || w.closedRequestBodyEarly() {
c.closeWriteAndWait() c.closeWriteAndWait()
} }
return return

107
src/net/http/transfer.go
View file

@ -809,17 +809,17 @@ func fixTrailer(header Header, chunked bool) (Header, error) {
// Close ensures that the body has been fully read // Close ensures that the body has been fully read
// and then reads the trailer if necessary. // and then reads the trailer if necessary.
type body struct { type body struct {
src io.Reader src io.Reader
hdr any // non-nil (Response or Request) value means read trailer hdr any // non-nil (Response or Request) value means read trailer
r *bufio.Reader // underlying wire-format reader for the trailer r *bufio.Reader // underlying wire-format reader for the trailer
closing bool // is the connection to be closed after reading body? 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 mu sync.Mutex // guards following, and calls to Read and Close
sawEOF bool sawEOF bool
closed bool closed bool
incompleteDiscard bool // if true, we failed to discard the body content completely earlyClose bool // Close called and we didn't read to the end of src
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
onHitEOF func() // if non-nil, func to call when EOF is Read
} }
// ErrBodyReadAfterClose is returned when reading a [Request] or [Response] // ErrBodyReadAfterClose is returned when reading a [Request] or [Response]
@ -968,69 +968,51 @@ func (b *body) unreadDataSizeLocked() int64 {
return -1 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 { func (b *body) Close() error {
b.mu.Lock() b.mu.Lock()
defer b.mu.Unlock() defer b.mu.Unlock()
if b.closed { if b.closed {
return nil return nil
} }
b.closed = true var err error
if !b.shouldDiscardBodyLocked() || b.inRequestHandler { switch {
return nil 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 b.closed = true
// the trailer headers after the body, if present.
_, err := io.Copy(io.Discard, bodyLocked{b})
return err return err
} }
func (b *body) didIncompleteDiscard() bool { func (b *body) didEarlyClose() bool {
b.mu.Lock() b.mu.Lock()
defer b.mu.Unlock() defer b.mu.Unlock()
return b.incompleteDiscard return b.earlyClose
} }
// bodyRemains reports whether future Read calls might // bodyRemains reports whether future Read calls might
@ -1054,6 +1036,9 @@ type bodyLocked struct {
} }
func (bl bodyLocked) Read(p []byte) (n int, err error) { func (bl bodyLocked) Read(p []byte) (n int, err error) {
if bl.b.closed {
return 0, ErrBodyReadAfterClose
}
return bl.b.readLocked(p) return bl.b.readLocked(p)
} }