code review 3395042: bufio: ensure that Read calls underlying Read once only.

bufio: ensure that Read calls underlying Read once only.

[rog, 2010-12-02 19:49:11]

Hello rsc (cc: golang-dev@googlegroups.com),

I'd like you to review this change.

[rog, 2010-12-02 19:50:15]

submitted without ReadFull. if you still think it needs ReadFull,
then i'll add it.

On 2 December 2010 19:49,  <rogpeppe@gmail.com> wrote:
> Reviewers: rsc,
>
> Message:
> Hello rsc (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change.
>
>
> Description:
> bufio: ensure that Read calls underlying Read once only.
>
> Please review this at http://codereview.appspot.com/3395042/
>
> Affected files:
>  M src/pkg/bufio/bufio.go
>
>
> Index: src/pkg/bufio/bufio.go
> ===================================================================
> --- a/src/pkg/bufio/bufio.go
> +++ b/src/pkg/bufio/bufio.go
> @@ -128,43 +128,41 @@
>
>  // Read reads data into p.
>  // It returns the number of bytes read into p.
> -// If nn < len(p), also returns an error explaining
> -// why the read is short.  At EOF, the count will be
> -// zero and err will be os.EOF.
> -func (b *Reader) Read(p []byte) (nn int, err os.Error) {
> -       nn = 0
> -       for len(p) > 0 {
> -               n := len(p)
> -               if b.w == b.r {
> -                       if b.err != nil {
> -                               return nn, b.err
> +// It calls Read at most once on the underlying Reader,
> +// hence n may be less than len(p).
> +// At EOF, the count will be zero and err will be os.EOF.
> +func (b *Reader) Read(p []byte) (n int, err os.Error) {
> +       n = len(p)
> +       if n == 0 {
> +               return 0, b.err
> +       }
> +       for b.w == b.r {
> +               if b.err != nil {
> +                       return 0, b.err
> +               }
> +               if len(p) >= len(b.buf) {
> +                       // Large read, empty buffer.
> +                       // Read directly into p to avoid copy.
> +                       n, b.err = b.rd.Read(p)
> +                       if n > 0 {
> +                               b.lastByte = int(p[n-1])
> +                               b.lastRuneSize = -1
>                        }
> -                       if len(p) >= len(b.buf) {
> -                               // Large read, empty buffer.
> -                               // Read directly into p to avoid copy.
> -                               n, b.err = b.rd.Read(p)
> -                               if n > 0 {
> -                                       b.lastByte = int(p[n-1])
> -                                       b.lastRuneSize = -1
> -                               }
> -                               p = p[n:]
> -                               nn += n
> -                               continue
> -                       }
> -                       b.fill()
> -                       continue
> +                       p = p[n:]
> +                       return n, b.err
>                }
> -               if n > b.w-b.r {
> -                       n = b.w - b.r
> -               }
> -               copy(p[0:n], b.buf[b.r:])
> -               p = p[n:]
> -               b.r += n
> -               b.lastByte = int(b.buf[b.r-1])
> -               b.lastRuneSize = -1
> -               nn += n
> +               b.fill()
>        }
> -       return nn, nil
> +
> +       if n > b.w-b.r {
> +               n = b.w - b.r
> +       }
> +       copy(p[0:n], b.buf[b.r:])
> +       p = p[n:]
> +       b.r += n
> +       b.lastByte = int(b.buf[b.r-1])
> +       b.lastRuneSize = -1
> +       return n, nil
>  }
>
>  // ReadByte reads and returns a single byte.
>
>
>

[rsc1, 2010-12-06 19:43:33]

http://codereview.appspot.com/3395042/diff/2001/src/pkg/bufio/bufio.go
File src/pkg/bufio/bufio.go (right):

http://codereview.appspot.com/3395042/diff/2001/src/pkg/bufio/bufio.go#newcod...
src/pkg/bufio/bufio.go:139: for b.w == b.r {
s/for/if/

[rog, 2010-12-07 10:37:53]

On 6 December 2010 19:43,  <rsc@google.com> wrote:
>
http://codereview.appspot.com/3395042/diff/2001/src/pkg/bufio/bufio.go#newcod...
> src/pkg/bufio/bufio.go:139: for b.w == b.r {
> s/for/if/

it's right as it is, i think.

if all the current buffer has been consumed, we're at the end of the
file, but we haven't got eof, the loop will be entered, fill will be
called, but still b.w == b.r, so the loop repeats and the return
clause is triggered because err != nil.

we could make it an if and put another if b.w == b.r && b.err != nil
after it, but that amounts to the same thing with more code.

[rsc, 2010-12-07 18:39:52]

That assumes that zero-length reads with err == nil cannot happen.
I'd rather write the code so that it works with them, so I guess
that means checking r==w && err after the loop.

[rog, 2010-12-07 18:52:32]

i thought you might say that...
FWIW, the original loop would loop indefinitely as
long as the underlying reader returns n==0 and err==nil.
i'm not sure it's the wrong behaviour either - some
network protocols can return zero-length packets
without implying EOF, and in this case looping could
be deemed appropriate.

however, that would break the one-call-to-the-underlying-reader
contract, so i find i've come around to your way of thinking.

good.

PTAL

On 7 December 2010 18:39,  <rsc@golang.org> wrote:
> That assumes that zero-length reads with err == nil cannot happen.
> I'd rather write the code so that it works with them, so I guess
> that means checking r==w && err after the loop.
>
>
> http://codereview.appspot.com/3395042/
>

[rsc, 2010-12-07 19:52:43]

On Tue, Dec 7, 2010 at 13:52, roger peppe <rogpeppe@gmail.com> wrote:
> i thought you might say that...
> FWIW, the original loop would loop indefinitely as
> long as the underlying reader returns n==0 and err==nil.
> i'm not sure it's the wrong behaviour either - some
> network protocols can return zero-length packets
> without implying EOF, and in this case looping could
> be deemed appropriate.

i think that was correct behavior for ReadFull.  :-)

russ

[rsc1, 2010-12-07 19:53:04]

LGTM

[rsc, 2010-12-07 19:54:18]

*** Submitted as 3decb5689887 ***

bufio: make Reader.Read implement io.Reader semantics

R=rsc
CC=golang-dev
http://codereview.appspot.com/3395042

Committer: Russ Cox <rsc@golang.org>