bufio: make Writer.ReadFrom not flush prematurely. For example,

many small writes to a network may be less efficient that a few
large writes.

This fixes net/http's TestClientWrites, broken by 6565056 that
introduced Writer.ReadFrom. That test implicitly assumed that
calling io.Copy on a *bufio.Writer wouldn't write to the
underlying network until the buffer was full.

R=dsymonds
CC=bradfitz, golang-dev, mchaten, mikioh.mikioh
https://golang.org/cl/6743044

diff --git a/src/pkg/bufio/bufio.go b/src/pkg/bufio/bufio.go
index d1c5a13bcab4758af03de7e7eb9f07d871541303..cd51585f847d5600baa0f985ab26356fd253f694 100644 (file)
--- a/src/pkg/bufio/bufio.go
+++ b/src/pkg/bufio/bufio.go
@@ -569,11 +569,10 @@ func (b *Writer) WriteString(s string) (int, error) {
 
 // ReadFrom implements io.ReaderFrom.
 func (b *Writer) ReadFrom(r io.Reader) (n int64, err error) {
-       if err = b.Flush(); err != nil {
-               return 0, err
-       }
-       if w, ok := b.wr.(io.ReaderFrom); ok {
-               return w.ReadFrom(r)
+       if b.Buffered() == 0 {
+               if w, ok := b.wr.(io.ReaderFrom); ok {
+                       return w.ReadFrom(r)
+               }
        }
        var m int
        for {
@@ -583,8 +582,10 @@ func (b *Writer) ReadFrom(r io.Reader) (n int64, err error) {
                }
                b.n += m
                n += int64(m)
-               if err1 := b.Flush(); err1 != nil {
-                       return n, err1
+               if b.Available() == 0 {
+                       if err1 := b.Flush(); err1 != nil {
+                               return n, err1
+                       }
                }
                if err != nil {
                        break

diff --git a/src/pkg/bufio/bufio_test.go b/src/pkg/bufio/bufio_test.go
index 3d07639e2aa704fbd5e73fd40f967029d2f39fab..763b12326aeacde2f38d42b9dced84fd518b048e 100644 (file)
--- a/src/pkg/bufio/bufio_test.go
+++ b/src/pkg/bufio/bufio_test.go
@@ -881,6 +881,64 @@ func TestWriterReadFromErrors(t *testing.T) {
        }
 }
 
+// TestWriterReadFromCounts tests that using io.Copy to copy into a
+// bufio.Writer does not prematurely flush the buffer. For example, when
+// buffering writes to a network socket, excessive network writes should be
+// avoided.
+func TestWriterReadFromCounts(t *testing.T) {
+       var w0 writeCountingDiscard
+       b0 := NewWriterSize(&w0, 1234)
+       b0.WriteString(strings.Repeat("x", 1000))
+       if w0 != 0 {
+               t.Fatalf("write 1000 'x's: got %d writes, want 0", w0)
+       }
+       b0.WriteString(strings.Repeat("x", 200))
+       if w0 != 0 {
+               t.Fatalf("write 1200 'x's: got %d writes, want 0", w0)
+       }
+       io.Copy(b0, &onlyReader{strings.NewReader(strings.Repeat("x", 30))})
+       if w0 != 0 {
+               t.Fatalf("write 1230 'x's: got %d writes, want 0", w0)
+       }
+       io.Copy(b0, &onlyReader{strings.NewReader(strings.Repeat("x", 9))})
+       if w0 != 1 {
+               t.Fatalf("write 1239 'x's: got %d writes, want 1", w0)
+       }
+
+       var w1 writeCountingDiscard
+       b1 := NewWriterSize(&w1, 1234)
+       b1.WriteString(strings.Repeat("x", 1200))
+       b1.Flush()
+       if w1 != 1 {
+               t.Fatalf("flush 1200 'x's: got %d writes, want 1", w1)
+       }
+       b1.WriteString(strings.Repeat("x", 89))
+       if w1 != 1 {
+               t.Fatalf("write 1200 + 89 'x's: got %d writes, want 1", w1)
+       }
+       io.Copy(b1, &onlyReader{strings.NewReader(strings.Repeat("x", 700))})
+       if w1 != 1 {
+               t.Fatalf("write 1200 + 789 'x's: got %d writes, want 1", w1)
+       }
+       io.Copy(b1, &onlyReader{strings.NewReader(strings.Repeat("x", 600))})
+       if w1 != 2 {
+               t.Fatalf("write 1200 + 1389 'x's: got %d writes, want 2", w1)
+       }
+       b1.Flush()
+       if w1 != 3 {
+               t.Fatalf("flush 1200 + 1389 'x's: got %d writes, want 3", w1)
+       }
+}
+
+// A writeCountingDiscard is like ioutil.Discard and counts the number of times
+// Write is called on it.
+type writeCountingDiscard int
+
+func (w *writeCountingDiscard) Write(p []byte) (int, error) {
+       *w++
+       return len(p), nil
+}
+
 // An onlyReader only implements io.Reader, no matter what other methods the underlying implementation may have.
 type onlyReader struct {
        r io.Reader