code review 7309063: io: fix CopyN EOF behavior, and adds a new CopyN test. (issue 7309063)

[brad...@golang.org]

Reviewers: golang-dev_googlegroups.com,

Message:
Hello golan...@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg/


Description:
io: fix CopyN EOF behavior, and adds a new CopyN test.

I'm not sure whether this is gross or not. Arguably, sendfile
in pkg net already had knowledge of io.CopyN and its
LimitedReader before. This keeps that knowledge, but changes
it slightly.

This CL doesn't update any OS other than Linux yet.

Please review this at https://codereview.appspot.com/7309063/

Affected files:
M src/pkg/io/io.go
M src/pkg/io/io_test.go
M src/pkg/net/sendfile_linux.go


Index: src/pkg/io/io.go
===================================================================
--- a/src/pkg/io/io.go
+++ b/src/pkg/io/io.go
@@ -299,14 +299,31 @@
// If dst implements the ReaderFrom interface,
// the copy is implemented using it.
func CopyN(dst Writer, src Reader, n int64) (written int64, err error) {
- written, err = Copy(dst, LimitReader(src, n))
- if written < n && err == nil {
- // src stopped early; must have been EOF.
+ r := &trackEOFReader{r: src}
+ written, err = Copy(dst, LimitReader(r, n))
+ if err == nil && r.sawEOF {
err = EOF
}
return
}

+type trackEOFReader struct {
+ r Reader
+ sawEOF bool
+}
+
+func (r *trackEOFReader) Read(p []byte) (n int, err error) {
+ n, err = r.r.Read(p)
+ if err == EOF {
+ r.sawEOF = true
+ }
+ return
+}
+
+func (r *trackEOFReader) WrappedReader() Reader {
+ return r.r
+}
+
// Copy copies from src to dst until either EOF is reached
// on src or an error occurs. It returns the number of bytes
// copied and the first error encountered while copying, if any.
Index: src/pkg/io/io_test.go
===================================================================
--- a/src/pkg/io/io_test.go
+++ b/src/pkg/io/io_test.go
@@ -81,6 +81,12 @@
}
}

+type eofReader struct{}
+
+func (f eofReader) Read(p []byte) (n int, err error) {
+ return len(p), EOF
+}
+
type noReadFrom struct {
w Writer
}
@@ -114,6 +120,11 @@
if n != 3 || err != EOF {
t.Errorf("CopyN(bytes.Buffer, foo, 4) = %d, %v; want 3, EOF", n, err)
}
+
+ n, err = CopyN(b, eofReader{}, 5)
+ if n != 5 || err != EOF {
+ t.Errorf("CopyN(bytes.Buffer, eofReader, 5) = %d, %v; want 5, EOF", n,
err)
+ }
}

func TestReadAtLeast(t *testing.T) {
Index: src/pkg/net/sendfile_linux.go
===================================================================
--- a/src/pkg/net/sendfile_linux.go
+++ b/src/pkg/net/sendfile_linux.go
@@ -31,6 +31,14 @@
return 0, nil, true
}
}
+ // Implemented by io.CopyN:
+ type wrapper interface {
+ WrappedReader() io.Reader
+ }
+ if wi, ok := r.(wrapper); ok {
+ r = wi.WrappedReader()
+ }
+
f, ok := r.(*os.File)
if !ok {
return 0, nil, false

[Russ Cox]

Counter-argument: leave new behavior alone and document, like we did for io.ReadFull.

[Brad Fitzpatrick]

Great, that's my preference too.

Actually depending on how I squint, it looks like the new behavior even still could fit the old docs.

It could be more clear, though.  I'll do that later, if nobody beats me to it.

[Brad Fitzpatrick]

Doc proposal: https://codereview.appspot.com/7314069