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

150
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
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,
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,
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,
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,
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,
bodyChunked: false, // has a Content-Length
reqConnClose: false,
shouldDiscardBody: 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,
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,
},
// 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.
7: {
bodySize: 1 << 20,
bodyChunked: true,
reqConnClose: true,
shouldDiscardBody: true,
wantEOFSearch: true,
wantNextReq: false,
},
// Big with Connection: close, so don't do any reads on Close.
// With Content-Length.
7: {
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()
numReqs++
if numReqs == 1 {
size0 = readBufLen()
req.Body.Close()
closedSize = readBufLen()
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)
}
}

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.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

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

@ -813,12 +813,12 @@ type body struct {
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
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
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
}
@ -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})
_, 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)
}