}
}
-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 {
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 {
}
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)
}
}
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()
- // 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()
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() {
// 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.didIncompleteDiscard() {
+ if w.requestBodyLimitHit || w.closedRequestBodyEarly() {
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?
-
- 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]
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
}
func (bl bodyLocked) Read(p []byte) (n int, err error) {
+ if bl.b.closed {
+ return 0, ErrBodyReadAfterClose
+ }
return bl.b.readLocked(p)
}