code review 5563045: bytes.Buffer: restore panic on out-of-memory

bytes.Buffer: restore panic on out-of-memory
Make the panic detectable, and use that in ioutil.ReadFile to
give an error if the file is too big.

[r, 2012-01-21 17:38:35]

Hello golang-dev@googlegroups.com,

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

[minux1, 2012-01-21 17:43:28]

http://codereview.appspot.com/5563045/diff/5/src/pkg/bytes/buffer_test.go
File src/pkg/bytes/buffer_test.go (right):

http://codereview.appspot.com/5563045/diff/5/src/pkg/bytes/buffer_test.go#new...
src/pkg/bytes/buffer_test.go:408: t.Error("error expected")
s/error/panic/ ?

[bradfitz, 2012-01-21 17:45:29]

LGTM
 On Jan 21, 2012 11:38 AM, <r@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:
> bytes.Buffer: restore panic on out-of-memory
> Make the panic detectable, and use that in ioutil.ReadFile to
> give an error if the file is too big.
>
> Please review this at
http://codereview.appspot.com/**5563045/<http://codereview.appspot.com/5563045/>
>
> Affected files:
>  M src/pkg/bytes/buffer.go
>  M src/pkg/bytes/buffer_test.go
>  M src/pkg/io/ioutil/ioutil.go
>
>
> Index: src/pkg/bytes/buffer.go
> ==============================**==============================**=======
> --- a/src/pkg/bytes/buffer.go
> +++ b/src/pkg/bytes/buffer.go
> @@ -33,7 +33,7 @@
>        opRead                   // Any other read operation.
>  )
>
> -// ErrTooLarge is returned if there is too much data to fit in a buffer.
> +// ErrTooLarge is passed to panic if memory cannot be allocated to store
> data in a buffer.
>  var ErrTooLarge = errors.New("bytes.Buffer: too large")
>
>  // Bytes returns a slice of the contents of the unread portion of the
> buffer;
> @@ -73,8 +73,7 @@
>
>  // grow grows the buffer to guarantee space for n more bytes.
>  // It returns the index where bytes should be written.
> -// If the buffer can't grow, it returns -1, which will
> -// become ErrTooLarge in the caller.
> +// If the buffer can't grow it will panic with ErrTooLarge.
>  func (b *Buffer) grow(n int) int {
>        m := b.Len()
>        // If buffer is empty, reset to recover space.
> @@ -88,9 +87,6 @@
>                } else {
>                        // not enough space anywhere
>                        buf = makeSlice(2*cap(b.buf) + n)
> -                       if buf == nil {
> -                               return -1
> -                       }
>                        copy(buf, b.buf[b.off:])
>                }
>                b.buf = buf
> @@ -102,6 +98,8 @@
>
>  // Write appends the contents of p to the buffer.  The return
>  // value n is the length of p; err is always nil.
> +// If the buffer becomes too large, Write will panic with
> +// ErrTooLarge.
>  func (b *Buffer) Write(p []byte) (n int, err error) {
>        b.lastRead = opInvalid
>        m := b.grow(len(p))
> @@ -146,9 +144,6 @@
>                                // not enough space using beginning of
> buffer;
>                                // double buffer capacity
>                                newBuf = makeSlice(2*cap(b.buf) + MinRead)
> -                               if newBuf == nil {
> -                                       return n, ErrTooLarge
> -                               }
>                        }
>                        copy(newBuf, b.buf[b.off:])
>                        b.buf = newBuf[:len(b.buf)-b.off]
> @@ -167,14 +162,14 @@
>        return n, nil // err is EOF, so return nil explicitly
>  }
>
> -// makeSlice allocates a slice of size n, returning nil if the slice
> cannot be allocated.
> +// makeSlice allocates a slice of size n. If the allocation fails, it
> panics
> +// with ErrTooLarge.
>  func makeSlice(n int) []byte {
> -       if n < 0 {
> -               return nil
> -       }
> -       // Catch out of memory panics.
> +       // If the make fails, give a known error.
>        defer func() {
> -               recover()
> +               if recover() != nil {
> +                       panic(ErrTooLarge)
> +               }
>        }()
>        return make([]byte, n)
>  }
> Index: src/pkg/bytes/buffer_test.go
> ==============================**==============================**=======
> --- a/src/pkg/bytes/buffer_test.go
> +++ b/src/pkg/bytes/buffer_test.go
> @@ -392,13 +392,18 @@
>        if testing.Short() {
>                return
>        }
> +       // We expect a panic.
> +       defer func() {
> +               if err, ok := recover().(error); ok && err == ErrTooLarge {
> +                       return
> +               } else {
> +                       t.Error(`expected "too large" error; got`, err)
> +               }
> +       }()
>        b := new(Buffer)
>        big := make([]byte, 500e6)
>        for i := 0; i < 1000; i++ {
> -               if _, err := b.Write(big); err != nil {
> -                       // Got error as expected. Stop
> -                       return
> -               }
> +               b.Write(big)
>        }
>        t.Error("error expected")
>  }
> Index: src/pkg/io/ioutil/ioutil.go
> ==============================**==============================**=======
> --- a/src/pkg/io/ioutil/ioutil.go
> +++ b/src/pkg/io/ioutil/ioutil.go
> @@ -14,9 +14,22 @@
>
>  // readAll reads from r until an error or EOF and returns the data it read
>  // from the internal buffer allocated with a specified capacity.
> -func readAll(r io.Reader, capacity int64) ([]byte, error) {
> +func readAll(r io.Reader, capacity int64) (b []byte, err error) {
>        buf := bytes.NewBuffer(make([]byte, 0, capacity))
> -       _, err := buf.ReadFrom(r)
> +       // If the buffer overflows, we will get bytes.ErrTooLarge.
> +       // Return that as an error. Any other panic remains.
> +       defer func() {
> +               e := recover()
> +               if e == nil {
> +                       return
> +               }
> +               if panicErr, ok := recover().(error); ok && panicErr ==
> bytes.ErrTooLarge {
> +                       err = panicErr
> +               } else {
> +                       panic(e)
> +               }
> +       }()
> +       _, err = buf.ReadFrom(r)
>        return buf.Bytes(), err
>  }
>
>
>
>

[r, 2012-01-21 17:47:03]

*** Submitted as b372a927701e ***

bytes.Buffer: restore panic on out-of-memory
Make the panic detectable, and use that in ioutil.ReadFile to
give an error if the file is too big.

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

[gri, 2012-01-21 17:50:49]

I am not convinced by this CL.

It seems to me that Write was the only issue; and so only Write would have to
throw the error. Everything else could have remained the same.

Am I missing something?
- gri

http://codereview.appspot.com/5563045/diff/5/src/pkg/bytes/buffer.go
File src/pkg/bytes/buffer.go (right):

http://codereview.appspot.com/5563045/diff/5/src/pkg/bytes/buffer.go#newcode146
src/pkg/bytes/buffer.go:146: newBuf = makeSlice(2*cap(b.buf) + MinRead)
shouldn't the makeSlice panic be caught in here instead of the ioutil function?

http://codereview.appspot.com/5563045/diff/5/src/pkg/io/ioutil/ioutil.go
File src/pkg/io/ioutil/ioutil.go (right):

http://codereview.appspot.com/5563045/diff/5/src/pkg/io/ioutil/ioutil.go#newc...
src/pkg/io/ioutil/ioutil.go:19: // If the buffer overflows, we will get
bytes.ErrTooLarge.
shouldn't this panic be handled in ReadFrom?

[rsc, 2012-01-21 17:59:26]

b.ReadFrom(r) is essentially defined as a shortcut for
r.Read(tmp)+b.Write(tmp).  It should behave the same as Write.

Russ

[fw, 2012-01-22 21:48:58]

http://codereview.appspot.com/5563045/diff/7/src/pkg/bytes/buffer.go
File src/pkg/bytes/buffer.go (right):

http://codereview.appspot.com/5563045/diff/7/src/pkg/bytes/buffer.go#newcode171
src/pkg/bytes/buffer.go:171: panic(ErrTooLarge)
I don't think this is thread-safe.  (The old code had this problem, too.)

Unfortunately, it is really hard to attribute memory allocation failures to the
guilty party.

[r, 2012-01-22 21:59:02]

http://codereview.appspot.com/5563045/diff/7/src/pkg/bytes/buffer.go
File src/pkg/bytes/buffer.go (right):

http://codereview.appspot.com/5563045/diff/7/src/pkg/bytes/buffer.go#newcode171
src/pkg/bytes/buffer.go:171: panic(ErrTooLarge)
In what sense is it not thread safe? The allocation failed, so this thread is
unhappy anyway.

[fw, 2012-01-23 07:17:28]

http://codereview.appspot.com/5563045/diff/7/src/pkg/bytes/buffer.go
File src/pkg/bytes/buffer.go (right):

http://codereview.appspot.com/5563045/diff/7/src/pkg/bytes/buffer.go#newcode171
src/pkg/bytes/buffer.go:171: panic(ErrTooLarge)
On 2012/01/22 21:59:02, r wrote:
> In what sense is it not thread safe? The allocation failed, so this thread is
> unhappy anyway.

Returning the error attributes the cause to the buffer operation.  But that's
not necessarily the case if another thread uses up the heap in parallel.  True,
the allocation has failed, but does this really convey any information to the
caller in such a case?

[r2, 2012-01-23 07:32:07]

On Jan 22, 2012, at 11:17 PM, fw@deneb.enyo.de wrote:

> 
> http://codereview.appspot.com/5563045/diff/7/src/pkg/bytes/buffer.go
> File src/pkg/bytes/buffer.go (right):
> 
>
http://codereview.appspot.com/5563045/diff/7/src/pkg/bytes/buffer.go#newcode171
> src/pkg/bytes/buffer.go:171: panic(ErrTooLarge)
> On 2012/01/22 21:59:02, r wrote:
>> In what sense is it not thread safe? The allocation failed, so this
> thread is
>> unhappy anyway.
> 
> Returning the error attributes the cause to the buffer operation.  But
> that's not necessarily the case if another thread uses up the heap in
> parallel.  True, the allocation has failed, but does this really convey
> any information to the caller in such a case?

Errors aren't about cause, they're about facts. The fact is the allocation
failed. There's little else to do in any case.

-rob