Skip to content

Commit

Permalink
net/http: refactor ResponseWriter.ReadFrom to permit splice on Linux
Browse files Browse the repository at this point in the history
Issue golang#40888 describes my motivation for this patch:

. Rather than probe and guess if sendfile will work, fix the underlying
  issue of starting to respond before src is readable

. Do not send a status OK if header has not yet been written and reading
  from src is destined to fail

. Implicitly takes care of needsSniff() case

This allows splice to work on linux when src is a socket or other
spliceable non regular file.

The extra read of 512 bytes may raise an objection, and that's fair, but
we're already swapping some syscall prep work for another and a read of
512 probably will not impact the overall performance. For shorter
bodies, there's likely less setup time. A little initial slop is not that
unusual in zero copy network code, and sometimes actually helps.

review feedback: use copyBufPool in both cases

Use the same pool for the normal copy case as we do for the short read.

Clean up and clarify comments.

clean up code addressing feedback

pass sendfile test

the sendfile path will require >512 bytes to kick in, so send a 1024
byte file for this test.

obey copy semantics

pattern the initial read and write after io.Copy(), paying attention to
either side's error and writing what has been read regardless.

This could be done with an io.LimitedReader with io.Copy() except that
returns the total bytes written, while a ReaderFrom needs to return the
total bytes read. While we could adjust for this by looking at the
limited reader's state, being explicit this way is probably more clear.

create sendfile test data in temp dir

Remove accidentally added 1024k file which was meant to be a 1024b file.
Generate the test data in a temporary directory regardless.
  • Loading branch information
PaulForgey committed Aug 26, 2020
1 parent b1253d2 commit 5459146
Show file tree
Hide file tree
Showing 2 changed files with 52 additions and 36 deletions.
18 changes: 16 additions & 2 deletions src/net/http/fs_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1136,6 +1136,20 @@ func TestLinuxSendfile(t *testing.T) {
t.Skipf("skipping; failed to run strace: %v", err)
}

filename := fmt.Sprintf("1kb-%d", os.Getpid())
filepath := path.Join(os.TempDir(), filename)
f, err := os.Create(filepath)
if err != nil {
t.Fatal(err)
}
err = f.Truncate(1024 * 3)
f.Sync()
f.Close()
if err != nil {
t.Fatal(err)
}
defer os.Remove(filepath)

var buf bytes.Buffer
child := exec.Command("strace", "-f", "-q", os.Args[0], "-test.run=TestLinuxSendfileChild")
child.ExtraFiles = append(child.ExtraFiles, lnf)
Expand All @@ -1146,7 +1160,7 @@ func TestLinuxSendfile(t *testing.T) {
t.Skipf("skipping; failed to start straced child: %v", err)
}

res, err := Get(fmt.Sprintf("http://%s/", ln.Addr()))
res, err := Get(fmt.Sprintf("http://%s/%s", ln.Addr(), filename))
if err != nil {
t.Fatalf("http client error: %v", err)
}
Expand Down Expand Up @@ -1192,7 +1206,7 @@ func TestLinuxSendfileChild(*testing.T) {
panic(err)
}
mux := NewServeMux()
mux.Handle("/", FileServer(Dir("testdata")))
mux.Handle("/", FileServer(Dir(os.TempDir())))
mux.HandleFunc("/quit", func(ResponseWriter, *Request) {
os.Exit(0)
})
Expand Down
70 changes: 36 additions & 34 deletions src/net/http/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -561,51 +561,53 @@ type writerOnly struct {
io.Writer
}

func srcIsRegularFile(src io.Reader) (isRegular bool, err error) {
switch v := src.(type) {
case *os.File:
fi, err := v.Stat()
if err != nil {
return false, err
}
return fi.Mode().IsRegular(), nil
case *io.LimitedReader:
return srcIsRegularFile(v.R)
default:
return
}
}

// ReadFrom is here to optimize copying from an *os.File regular file
// to a *net.TCPConn with sendfile.
// to a *net.TCPConn with sendfile, or from a supported src type such
// as a *net.TCPConn on Linux with splice.
func (w *response) ReadFrom(src io.Reader) (n int64, err error) {
bufp := copyBufPool.Get().(*[]byte)
buf := *bufp
defer copyBufPool.Put(bufp)

// Our underlying w.conn.rwc is usually a *TCPConn (with its
// own ReadFrom method). If not, or if our src isn't a regular
// file, just fall back to the normal copy method.
// own ReadFrom method). If not, just fall back to the normal
// copy method.
rf, ok := w.conn.rwc.(io.ReaderFrom)
regFile, err := srcIsRegularFile(src)
if err != nil {
return 0, err
}
if !ok || !regFile {
bufp := copyBufPool.Get().(*[]byte)
defer copyBufPool.Put(bufp)
return io.CopyBuffer(writerOnly{w}, src, *bufp)
if !ok {
return io.CopyBuffer(writerOnly{w}, src, buf)
}

// sendfile path:

if !w.wroteHeader {
w.WriteHeader(StatusOK)
}
// Do not start actually writing response until src is readable.
// If body length is <= sniffLen, sendfile/splice path will do
// little anyway. This small read also satisfies sniffing the
// body in case Content-Type is missing.
nr, er := src.Read(buf[:sniffLen])
atEOF := errors.Is(er, io.EOF)
n += int64(nr)

if w.needsSniff() {
n0, err := io.Copy(writerOnly{w}, io.LimitReader(src, sniffLen))
n += n0
if err != nil {
return n, err
if nr > 0 {
// Write the small amount read normally.
nw, ew := w.Write(buf[:nr])
if ew != nil {
err = ew
} else if nr != nw {
err = io.ErrShortWrite
}
}
if err == nil && er != nil && !atEOF {
err = er
}

// Do not send StatusOK in the error case where nothing has been written.
if err == nil && !w.wroteHeader {
w.WriteHeader(StatusOK) // nr == 0, no error (or EOF)
}

if err != nil || atEOF {
return n, err
}

w.w.Flush() // get rid of any previous writes
w.cw.flush() // make sure Header is written; flush data to rwc
Expand Down

0 comments on commit 5459146

Please sign in to comment.