Skip to content

Commit cd35c99

Browse files
vancluevervancluever
committed
net/http: let Transport request body writes use sendfile
net.TCPConn has the ability to send data out using system calls such as sendfile when the source data comes from an *os.File. However, the way that I/O has been laid out in the transport means that the File is actually wrapped behind two outer io.Readers, and as such the TCP stack cannot properly type-assert the reader, ensuring that it falls back to genericReadFrom. This commit does the following: * Removes transferBodyReader and moves its functionality to a new doBodyCopy helper. This is not an io.Reader implementation, but no functionality is lost this way, and it allows us to unwrap one layer from the body. * The second layer of the body is unwrapped if the original reader was wrapped with ioutil.NopCloser, which is what NewRequest wraps the body in if it's not a ReadCloser on its own. The unwrap operation passes through the existing body if there's no nopCloser. Note that this depends on change https://golang.org/cl/163737 to properly function, as the lack of ReaderFrom implementation otherwise means that this functionality is essentially walled off. Updates #30377.
1 parent 91170d7 commit cd35c99

File tree

2 files changed

+222
-19
lines changed

2 files changed

+222
-19
lines changed

‎src/net/http/transfer.go

Lines changed: 34 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -53,19 +53,6 @@ func (br *byteReader) Read(p []byte) (n int, err error) {
5353
return 1, io.EOF
5454
}
5555

56-
// transferBodyReader is an io.Reader that reads from tw.Body
57-
// and records any non-EOF error in tw.bodyReadError.
58-
// It is exactly 1 pointer wide to avoid allocations into interfaces.
59-
type transferBodyReader struct{ tw *transferWriter }
60-
61-
func (br transferBodyReader) Read(p []byte) (n int, err error) {
62-
n, err = br.tw.Body.Read(p)
63-
if err != nil && err != io.EOF {
64-
br.tw.bodyReadError = err
65-
}
66-
return
67-
}
68-
6956
// transferWriter inspects the fields of a user-supplied Request or Response,
7057
// sanitizes them without changing the user object and provides methods for
7158
// writing the respective header, body and trailer in wire format.
@@ -347,15 +334,18 @@ func (t *transferWriter) writeBody(w io.Writer) error {
347334
var err error
348335
var ncopy int64
349336

350-
// Write body
337+
// Write body. We "unwrap" the body first if it was wrapped in a
338+
// nopCloser. This is to ensure that we can take advantage of
339+
// OS-level optimizations in the event that the body is an
340+
// *os.File.
351341
if t.Body != nil {
352-
var body = transferBodyReader{t}
342+
var body = t.unwrapBody()
353343
if chunked(t.TransferEncoding) {
354344
if bw, ok := w.(*bufio.Writer); ok && !t.IsResponse {
355345
w = &internal.FlushAfterChunkWriter{Writer: bw}
356346
}
357347
cw := internal.NewChunkedWriter(w)
358-
_, err = io.Copy(cw, body)
348+
_, err = t.doBodyCopy(cw, body)
359349
if err == nil {
360350
err = cw.Close()
361351
}
@@ -364,14 +354,14 @@ func (t *transferWriter) writeBody(w io.Writer) error {
364354
if t.Method == "CONNECT" {
365355
dst = bufioFlushWriter{dst}
366356
}
367-
ncopy, err = io.Copy(dst, body)
357+
ncopy, err = t.doBodyCopy(dst, body)
368358
} else {
369-
ncopy, err = io.Copy(w, io.LimitReader(body, t.ContentLength))
359+
ncopy, err = t.doBodyCopy(w, io.LimitReader(body, t.ContentLength))
370360
if err != nil {
371361
return err
372362
}
373363
var nextra int64
374-
nextra, err = io.Copy(ioutil.Discard, body)
364+
nextra, err = t.doBodyCopy(ioutil.Discard, body)
375365
ncopy += nextra
376366
}
377367
if err != nil {
@@ -402,6 +392,31 @@ func (t *transferWriter) writeBody(w io.Writer) error {
402392
return err
403393
}
404394

395+
// doBodyCopy wraps a copy operation, with any resulting error also
396+
// being saved in bodyReadError.
397+
//
398+
// This function is only intended for use in writeBody.
399+
func (t *transferWriter) doBodyCopy(dst io.Writer, src io.Reader) (n int64, err error) {
400+
n, err = io.Copy(dst, src)
401+
if err != nil && err != io.EOF {
402+
t.bodyReadError = err
403+
}
404+
return
405+
}
406+
407+
// unwrapBodyReader unwraps the body's inner reader if it's a
408+
// nopCloser. This is to ensure that body writes sourced from local
409+
// files (*os.File types) are properly optimized.
410+
//
411+
// This function is only intended for use in writeBody.
412+
func (t *transferWriter) unwrapBody() io.Reader {
413+
if reflect.TypeOf(t.Body) == nopCloserType {
414+
return reflect.ValueOf(t.Body).Field(0).Interface().(io.Reader)
415+
}
416+
417+
return t.Body
418+
}
419+
405420
type transferReader struct {
406421
// Input
407422
Header Header

‎src/net/http/transfer_test.go

Lines changed: 188 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,12 @@ package http
77
import (
88
"bufio"
99
"bytes"
10+
"crypto/rand"
11+
"fmt"
1012
"io"
1113
"io/ioutil"
14+
"os"
15+
"reflect"
1216
"strings"
1317
"testing"
1418
)
@@ -90,3 +94,187 @@ func TestDetectInMemoryReaders(t *testing.T) {
9094
}
9195
}
9296
}
97+
98+
type mockTransferWriter struct {
99+
CalledReader io.Reader
100+
WriteCalled bool
101+
}
102+
103+
var _ io.ReaderFrom = (*mockTransferWriter)(nil)
104+
105+
func (w *mockTransferWriter) ReadFrom(r io.Reader) (int64, error) {
106+
w.CalledReader = r
107+
return io.Copy(ioutil.Discard, r)
108+
}
109+
110+
func (w *mockTransferWriter) Write(p []byte) (int, error) {
111+
w.WriteCalled = true
112+
return ioutil.Discard.Write(p)
113+
}
114+
115+
func TestTransferWriterWriteBodyReaderTypes(t *testing.T) {
116+
fileType := reflect.TypeOf(&os.File{})
117+
bufferType := reflect.TypeOf(&bytes.Buffer{})
118+
119+
nBytes := int64(1 << 10)
120+
newFileFunc := func() (r io.Reader, done func(), err error) {
121+
f, err := ioutil.TempFile("", "net-http-newfilefunc")
122+
if err != nil {
123+
return nil, nil, err
124+
}
125+
126+
// Write some bytes to the file to enable reading.
127+
if _, err := io.CopyN(f, rand.Reader, nBytes); err != nil {
128+
return nil, nil, fmt.Errorf("failed to write data to file: %v", err)
129+
}
130+
if _, err := f.Seek(0, 0); err != nil {
131+
return nil, nil, fmt.Errorf("failed to seek to front: %v", err)
132+
}
133+
134+
done = func() {
135+
f.Close()
136+
os.Remove(f.Name())
137+
}
138+
139+
return f, done, nil
140+
}
141+
142+
newBufferFunc := func() (io.Reader, func(), error) {
143+
return bytes.NewBuffer(make([]byte, nBytes)), func() {}, nil
144+
}
145+
146+
cases := []struct {
147+
name string
148+
bodyFunc func() (io.Reader, func(), error)
149+
method string
150+
contentLength int64
151+
transferEncoding []string
152+
limitedReader bool
153+
expectedReader reflect.Type
154+
expectedWrite bool
155+
}{
156+
{
157+
name: "file, non-chunked, size set",
158+
bodyFunc: newFileFunc,
159+
method: "PUT",
160+
contentLength: nBytes,
161+
limitedReader: true,
162+
expectedReader: fileType,
163+
},
164+
{
165+
name: "file, non-chunked, size set, nopCloser wrapped",
166+
method: "PUT",
167+
bodyFunc: func() (io.Reader, func(), error) {
168+
r, cleanup, err := newFileFunc()
169+
return ioutil.NopCloser(r), cleanup, err
170+
},
171+
contentLength: nBytes,
172+
limitedReader: true,
173+
expectedReader: fileType,
174+
},
175+
{
176+
name: "file, non-chunked, negative size",
177+
method: "PUT",
178+
bodyFunc: newFileFunc,
179+
contentLength: -1,
180+
expectedReader: fileType,
181+
},
182+
{
183+
name: "file, non-chunked, CONNECT, negative size",
184+
method: "CONNECT",
185+
bodyFunc: newFileFunc,
186+
contentLength: -1,
187+
expectedReader: fileType,
188+
},
189+
{
190+
name: "file, chunked",
191+
method: "PUT",
192+
bodyFunc: newFileFunc,
193+
transferEncoding: []string{"chunked"},
194+
expectedWrite: true,
195+
},
196+
{
197+
name: "buffer, non-chunked, size set",
198+
bodyFunc: newBufferFunc,
199+
method: "PUT",
200+
contentLength: nBytes,
201+
limitedReader: true,
202+
expectedReader: bufferType,
203+
},
204+
{
205+
name: "buffer, non-chunked, size set, nopCloser wrapped",
206+
method: "PUT",
207+
bodyFunc: func() (io.Reader, func(), error) {
208+
r, cleanup, err := newBufferFunc()
209+
return ioutil.NopCloser(r), cleanup, err
210+
},
211+
contentLength: nBytes,
212+
limitedReader: true,
213+
expectedReader: bufferType,
214+
},
215+
{
216+
name: "buffer, non-chunked, negative size",
217+
method: "PUT",
218+
bodyFunc: newBufferFunc,
219+
contentLength: -1,
220+
expectedWrite: true,
221+
},
222+
{
223+
name: "buffer, non-chunked, CONNECT, negative size",
224+
method: "CONNECT",
225+
bodyFunc: newBufferFunc,
226+
contentLength: -1,
227+
expectedWrite: true,
228+
},
229+
{
230+
name: "buffer, chunked",
231+
method: "PUT",
232+
bodyFunc: newBufferFunc,
233+
transferEncoding: []string{"chunked"},
234+
expectedWrite: true,
235+
},
236+
}
237+
238+
for _, tc := range cases {
239+
t.Run(tc.name, func(t *testing.T) {
240+
body, cleanup, err := tc.bodyFunc()
241+
if err != nil {
242+
t.Fatal(err)
243+
}
244+
defer cleanup()
245+
246+
mw := &mockTransferWriter{}
247+
tw := &transferWriter{
248+
Body: body,
249+
ContentLength: tc.contentLength,
250+
TransferEncoding: tc.transferEncoding,
251+
}
252+
253+
if err := tw.writeBody(mw); err != nil {
254+
t.Fatal(err)
255+
}
256+
257+
if tc.expectedReader != nil {
258+
if mw.CalledReader == nil {
259+
t.Fatal("did not call ReadFrom")
260+
}
261+
262+
var actualReader reflect.Type
263+
lr, ok := mw.CalledReader.(*io.LimitedReader)
264+
if ok && tc.limitedReader {
265+
actualReader = reflect.TypeOf(lr.R)
266+
} else {
267+
actualReader = reflect.TypeOf(mw.CalledReader)
268+
}
269+
270+
if tc.expectedReader != actualReader {
271+
t.Fatalf("got reader %T want %T", actualReader, tc.expectedReader)
272+
}
273+
}
274+
275+
if tc.expectedWrite && !mw.WriteCalled {
276+
t.Fatal("did not invoke Write")
277+
}
278+
})
279+
}
280+
}

0 commit comments

Comments
 (0)