}
}
-type handlerBodyCloseTest struct {
+type bodyDiscardTest struct {
bodySize int
bodyChunked bool
reqConnClose bool
- wantEOFSearch bool // should Handler's Body.Close do Reads, looking for EOF?
- wantNextReq bool // should it find the next request on the same conn?
+ shouldDiscardBody bool // should the handler discard body after it exits?
}
-func (t handlerBodyCloseTest) connectionHeader() string {
+func (t bodyDiscardTest) connectionHeader() string {
if t.reqConnClose {
return "Connection: close\r\n"
}
return ""
}
-var handlerBodyCloseTests = [...]handlerBodyCloseTest{
- // Small enough to slurp past to the next request +
- // has Content-Length.
+var bodyDiscardTests = [...]bodyDiscardTest{
+ // Have:
+ // - Small body.
+ // - Content-Length defined.
+ // Should:
+ // - Discard remaining body.
0: {
- bodySize: 20 << 10,
- bodyChunked: false,
- reqConnClose: false,
- wantEOFSearch: true,
- wantNextReq: true,
+ bodySize: 20 << 10,
+ bodyChunked: false,
+ reqConnClose: false,
+ shouldDiscardBody: true,
},
- // Small enough to slurp past to the next request +
- // is chunked.
+ // Have:
+ // - Small body.
+ // - Chunked (no Content-Length defined).
+ // Should:
+ // - Discard remaining body.
1: {
- bodySize: 20 << 10,
- bodyChunked: true,
- reqConnClose: false,
- wantEOFSearch: true,
- wantNextReq: true,
+ bodySize: 20 << 10,
+ bodyChunked: true,
+ reqConnClose: false,
+ shouldDiscardBody: true,
},
- // Small enough to slurp past to the next request +
- // has Content-Length +
- // declares Connection: close (so pointless to read more).
+ // Have:
+ // - Small body.
+ // - Content-Length defined.
+ // - Connection: close.
+ // Should:
+ // - Not discard remaining body (no point as Connection: close).
2: {
- bodySize: 20 << 10,
- bodyChunked: false,
- reqConnClose: true,
- wantEOFSearch: false,
- wantNextReq: false,
+ bodySize: 20 << 10,
+ bodyChunked: false,
+ reqConnClose: true,
+ shouldDiscardBody: false,
},
- // 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.
+ // 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.
3: {
- bodySize: 20 << 10,
- bodyChunked: true,
- reqConnClose: true,
- wantEOFSearch: true,
- wantNextReq: false,
+ bodySize: 20 << 10,
+ bodyChunked: true,
+ reqConnClose: true,
+ shouldDiscardBody: true,
},
- // Big with Content-Length, so give up immediately if we know it's too big.
+ // Have:
+ // - Large body.
+ // - Content-Length defined.
+ // Should:
+ // - Not discard remaining body (we know it is too large from Content-Length).
4: {
- bodySize: 1 << 20,
- bodyChunked: false, // has a Content-Length
- reqConnClose: false,
- wantEOFSearch: false,
- wantNextReq: false,
+ bodySize: 1 << 20,
+ bodyChunked: false,
+ reqConnClose: false,
+ shouldDiscardBody: false,
},
- // Big chunked, so read a bit before giving up.
+ // Have:
+ // - Large body.
+ // - Chunked (no Content-Length defined).
+ // Should:
+ // - Discard remaining body (chunked, so we try up to a limit before giving up).
5: {
- bodySize: 1 << 20,
- bodyChunked: true,
- reqConnClose: false,
- wantEOFSearch: true,
- wantNextReq: false,
+ bodySize: 1 << 20,
+ bodyChunked: true,
+ reqConnClose: false,
+ shouldDiscardBody: true,
},
- // Big with Connection: close, but chunked, so search for trailers.
- // TODO: maybe skip this search if no trailers were declared
- // in the headers.
+ // Have:
+ // - Large body.
+ // - Content-Length defined.
+ // - Connection: close.
+ // Should:
+ // - Not discard remaining body (Connection: Close, and Content-Length is too large).
6: {
- bodySize: 1 << 20,
- bodyChunked: true,
- reqConnClose: true,
- wantEOFSearch: true,
- wantNextReq: false,
+ bodySize: 1 << 20,
+ bodyChunked: false,
+ reqConnClose: true,
+ shouldDiscardBody: false,
},
-
- // Big with Connection: close, so don't do any reads on Close.
- // With Content-Length.
+ // 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: false,
- reqConnClose: true,
- wantEOFSearch: false,
- wantNextReq: false,
+ bodySize: 1 << 20,
+ bodyChunked: true,
+ reqConnClose: true,
+ shouldDiscardBody: true,
},
}
-func TestHandlerBodyClose(t *testing.T) {
+func TestBodyDiscard(t *testing.T) {
setParallel(t)
if testing.Short() && testenv.Builder() == "" {
t.Skip("skipping in -short mode")
}
- for i, tt := range handlerBodyCloseTests {
- testHandlerBodyClose(t, i, tt)
+ for i, tt := range bodyDiscardTests {
+ testBodyDiscard(t, i, tt)
}
}
-func testHandlerBodyClose(t *testing.T, i int, tt handlerBodyCloseTest) {
+func testBodyDiscard(t *testing.T, i int, tt bodyDiscardTest) {
conn := new(testConn)
body := strings.Repeat("x", tt.bodySize)
if tt.bodyChunked {
cw.Close()
conn.readBuf.WriteString("\r\n")
} else {
- conn.readBuf.Write([]byte(fmt.Sprintf(
+ conn.readBuf.Write(fmt.Appendf(nil,
"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 {
}
ls := &oneConnListener{conn}
- var numReqs int
- var size0, size1 int
+ var initialSize, closedSize, exitSize int
go Serve(ls, HandlerFunc(func(rw ResponseWriter, req *Request) {
- numReqs++
- if numReqs == 1 {
- size0 = readBufLen()
- req.Body.Close()
- size1 = readBufLen()
- }
+ initialSize = readBufLen()
+ req.Body.Close()
+ closedSize = readBufLen()
}))
<-conn.closec
- if numReqs < 1 || numReqs > 2 {
- t.Fatalf("%d. bug in test. unexpected number of requests = %d", i, numReqs)
+ 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)
}
- 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 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)
+ 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)
}
}
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) {
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()
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() {
// 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()
w.finishRequest()
c.rwc.SetWriteDeadline(time.Time{})
if !w.shouldReuseConnection() {
- if w.requestBodyLimitHit || w.closedRequestBodyEarly() {
+ if w.requestBodyLimitHit || w.didIncompleteDiscard() {
c.closeWriteAndWait()
}
return
// 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?
- 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
+ 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
}
// ErrBodyReadAfterClose is returned when reading a [Request] or [Response]
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
}
- 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})
- }
b.closed = true
+ if !b.shouldDiscardBodyLocked() || b.inRequestHandler {
+ return nil
+ }
+ // 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})
return err
}
-func (b *body) didEarlyClose() bool {
+func (b *body) didIncompleteDiscard() bool {
b.mu.Lock()
defer b.mu.Unlock()
- return b.earlyClose
+ return b.incompleteDiscard
}
// bodyRemains reports whether future Read calls might
}
func (bl bodyLocked) Read(p []byte) (n int, err error) {
- if bl.b.closed {
- return 0, ErrBodyReadAfterClose
- }
return bl.b.readLocked(p)
}