code review 7235074: io: guarantee err == nil for full reads in ReadFull and...

io: guarantee err == nil for full reads in ReadFull and ReadAtLeast

This is a backwards compatible API change that fixes broken code.

In Go 1.0, ReadFull(r, buf) could return either len(buf), nil or len(buf), non-nil.
Most code expects only the former, so do that and document the guarantee.

Code that was correct before is still correct.
Code that was incorrect before, by assuming the guarantee, is now correct too.

The same applies to ReadAtLeast.

Fixes issue 4544.

[rsc, 2013-01-31 17:12:27]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go/

[bradfitz, 2013-01-31 17:23:48]

LGTM

On Thu, Jan 31, 2013 at 9:12 AM, <rsc@golang.org> wrote:

> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://code.google.com/p/go/
>
>
> Description:
> io: guarantee err == nil for full reads in ReadFull and ReadAtLeast
>
> This is a backwards compatible API change that fixes broken code.
>
> In Go 1.0, ReadFull(r, buf) could return either len(buf), nil or
> len(buf), non-nil.
> Most code expects only the former, so do that and document the
> guarantee.
>
> Code that was correct before is still correct.
> Code that was incorrect before, by assuming the guarantee, is now
> correct too.
>
> The same applies to ReadAtLeast.
>
> Fixes issue 4544.
>
> Please review this at
https://codereview.appspot.**com/7235074/<https://codereview.appspot.com/7235...
>
> Affected files:
>   M src/pkg/io/io.go
>   M src/pkg/io/io_test.go
>
>
> Index: src/pkg/io/io.go
> ==============================**==============================**=======
> --- a/src/pkg/io/io.go
> +++ b/src/pkg/io/io.go
> @@ -262,6 +262,7 @@
>  // If an EOF happens after reading fewer than min bytes,
>  // ReadAtLeast returns ErrUnexpectedEOF.
>  // If min is greater than the length of buf, ReadAtLeast returns
> ErrShortBuffer.
> +// On return, n >= min if and only if err == nil.
>  func ReadAtLeast(r Reader, buf []byte, min int) (n int, err error) {
>         if len(buf) < min {
>                 return 0, ErrShortBuffer
> @@ -271,12 +272,10 @@
>                 nn, err = r.Read(buf[n:])
>                 n += nn
>         }
> -       if err == EOF {
> -               if n >= min {
> -                       err = nil
> -               } else if n > 0 {
> -                       err = ErrUnexpectedEOF
> -               }
> +       if n >= min {
> +               err = nil
> +       } else if n > 0 && err == EOF {
> +               err = ErrUnexpectedEOF
>         }
>         return
>  }
> @@ -286,6 +285,7 @@
>  // The error is EOF only if no bytes were read.
>  // If an EOF happens after reading some but not all the bytes,
>  // ReadFull returns ErrUnexpectedEOF.
> +// On return, n == len(buf) if and only if err == nil.
>  func ReadFull(r Reader, buf []byte) (n int, err error) {
>         return ReadAtLeast(r, buf, len(buf))
>  }
> Index: src/pkg/io/io_test.go
> ==============================**==============================**=======
> --- a/src/pkg/io/io_test.go
> +++ b/src/pkg/io/io_test.go
> @@ -6,6 +6,7 @@
>
>  import (
>         "bytes"
> +       "fmt"
>         . "io"
>         "strings"
>         "testing"
> @@ -120,22 +121,30 @@
>         testReadAtLeast(t, &rb)
>  }
>
> -// A version of bytes.Buffer that returns n > 0, EOF on Read
> +// A version of bytes.Buffer that returns n > 0, err on Read
>  // when the input is exhausted.
> -type dataAndEOFBuffer struct {
> +type dataAndErrorBuffer struct {
> +       err error
>         bytes.Buffer
>  }
>
> -func (r *dataAndEOFBuffer) Read(p []byte) (n int, err error) {
> +func (r *dataAndErrorBuffer) Read(p []byte) (n int, err error) {
>         n, err = r.Buffer.Read(p)
>         if n > 0 && r.Buffer.Len() == 0 && err == nil {
> -               err = EOF
> +               err = r.err
>         }
>         return
>  }
>
>  func TestReadAtLeastWithDataAndEOF(**t *testing.T) {
> -       var rb dataAndEOFBuffer
> +       var rb dataAndErrorBuffer
> +       rb.err = EOF
> +       testReadAtLeast(t, &rb)
> +}
> +
> +func TestReadAtLeastWithDataAndErro**r(t *testing.T) {
> +       var rb dataAndErrorBuffer
> +       rb.err = fmt.Errorf("fake error")
>         testReadAtLeast(t, &rb)
>  }
>
> @@ -169,8 +178,12 @@
>         }
>         rb.Write([]byte("4"))
>         n, err = ReadAtLeast(rb, buf, 2)
> -       if err != ErrUnexpectedEOF {
> -               t.Errorf("expected ErrUnexpectedEOF, got %v", err)
> +       want := ErrUnexpectedEOF
> +       if rb, ok := rb.(*dataAndErrorBuffer); ok && rb.err != EOF {
> +               want = rb.err
> +       }
> +       if err != want {
> +               t.Errorf("expected %v, got %v", want, err)
>         }
>         if n != 1 {
>                 t.Errorf("expected to have read 1 bytes, got %v", n)
>
>
> --
>
> ---You received this message because you are subscribed to the Google
> Groups "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to
golang-dev+unsubscribe@**googlegroups.com<golang-dev%2Bunsubscribe@googlegrou...
> .
> For more options, visit
https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/o...
> .
>
>
>

[minux1, 2013-01-31 17:28:18]

awesome, thank you!

[rsc, 2013-01-31 21:47:10]

*** Submitted as https://code.google.com/p/go/source/detail?r=29bf5ff5064e ***

io: guarantee err == nil for full reads in ReadFull and ReadAtLeast

This is a backwards compatible API change that fixes broken code.

In Go 1.0, ReadFull(r, buf) could return either len(buf), nil or len(buf),
non-nil.
Most code expects only the former, so do that and document the guarantee.

Code that was correct before is still correct.
Code that was incorrect before, by assuming the guarantee, is now correct too.

The same applies to ReadAtLeast.

Fixes issue 4544.

R=golang-dev, bradfitz, minux.ma
CC=golang-dev
https://codereview.appspot.com/7235074

[dave_cheney.net, 2013-01-31 22:00:44]

There may have been a test regression due to this change. 

http://build.golang.org/log/f779a324b4d0c9ab6ed5074e6d355b8804f963c3

(And others)

On 01/02/2013, at 8:47, rsc@golang.org wrote:

> *** Submitted as
> https://code.google.com/p/go/source/detail?r=29bf5ff5064e ***
> 
> io: guarantee err == nil for full reads in ReadFull and ReadAtLeast
> 
> This is a backwards compatible API change that fixes broken code.
> 
> In Go 1.0, ReadFull(r, buf) could return either len(buf), nil or
> len(buf), non-nil.
> Most code expects only the former, so do that and document the
> guarantee.
> 
> Code that was correct before is still correct.
> Code that was incorrect before, by assuming the guarantee, is now
> correct too.
> 
> The same applies to ReadAtLeast.
> 
> Fixes issue 4544.
> 
> R=golang-dev, bradfitz, minux.ma
> CC=golang-dev
> https://codereview.appspot.com/7235074
> 
> 
> https://codereview.appspot.com/7235074/
> 
> -- 
> 
> ---You received this message because you are subscribed to the Google Groups
"golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email to golang-dev+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
> 
>