bufio: Reader.Peek should return EOF when there's nothing more to read

[welkeyever]

What version of Go are you using (go version)?

$ go version
go version go1.16.6 darwin/amd64

Does this issue reproduce with the latest release?

yes

What did you do?

// Peek returns the next n bytes without advancing the reader. The bytes stop
// being valid at the next read call. If Peek returns fewer than n bytes, it
// also returns an error explaining why the read is short. The error is
// ErrBufferFull if n is larger than b's buffer size.
//
// Calling Peek prevents a UnreadByte or UnreadRune call from succeeding
// until the next read operation.
func (b *Reader) Peek(n int) ([]byte, error) {
	if n < 0 {
		return nil, ErrNegativeCount
	}

	b.lastByte = -1
	b.lastRuneSize = -1

	for b.w-b.r < n && b.w-b.r < len(b.buf) && b.err == nil {
		b.fill() // b.w-b.r < len(b.buf) => buffer is not full
	}

	if n > len(b.buf) {
		return b.buf[b.r:b.w], ErrBufferFull
	}

	// 0 <= n <= len(b.buf)
	var err error
	if avail := b.w - b.r; avail < n {
		// not enough data in buffer
		n = avail
		err = b.readErr()
		if err == nil {
			err = ErrBufferFull
		}
	}
	return b.buf[b.r : b.r+n], err
}

In Peek(n), if n is larger than len(b.buf), it will only return the ErrBufferFull error even if the b.err == EOF.

What did you expect to see?

In this n is larger than len(b.buf) situation. EOF error may be more precise when outer logical need judge whether to do next by the error returning from Peek(n)？

mostly like:

for {
	b, err := buf.Peek(n)
	if err == EOF{
		break
	}
}

but if only return ErrBufferFull error here, it leads a forever loop ..

A POSSIPLE SOLUTION MAY:

func (b *Reader) Peek(n int) ([]byte, error) {
	if n < 0 {
		return nil, ErrNegativeCount
	}

	b.lastByte = -1
	b.lastRuneSize = -1

	for b.w-b.r < n && b.w-b.r < len(b.buf) && b.err == nil {
		b.fill() // b.w-b.r < len(b.buf) => buffer is not full
	}
	
	// check the b.err before returning ErrBufferFull directly.
	if n > len(b.buf) && b.err == nil {
		return b.buf[b.r:b.w], ErrBufferFull
	}

	// 0 <= n <= len(b.buf)
	var err error
	if avail := b.w - b.r; avail < n {
		// not enough data in buffer
		n = avail
		err = b.readErr()
		if err == nil {
			err = ErrBufferFull
		}
	}
	return b.buf[b.r : b.r+n], err
}

[ianlancetaylor]

I think the current documentation of the method is very clear: "The error is ErrBufferFull if n is larger than b's buffer size." Perhaps it would have been a good idea to return io.EOF in some cases, but changing that now means changing the existing documented behavior. I think we would need a very good reason to do that, and I'm not yet seeing that here.

The loop you wrote above doesn't make sense to me. Every time you call Peek you will get the same results. I don't see why anybody would call Peek in a loop.

[welkeyever]

yes I can see the documentation there. I wonder that whether it would be more handy if we check the b.err before directly returning ErrBufferFull?

the example is a simple sketch, but what I wanna say here is that, in this situation, an io.EOF error is more helpful than ErrBufferFull, as the caller will get the message that the ’no more data’ here not the endless ErrBufferFull.

[ianlancetaylor]

It would help us understand this issue if you could show an example of a loop that becomes endless due to returnin bufio.ErrBufferFull rather than io.EOF. Because right now I don't understand how that could happen. Thanks.

[gopherbot]

Timed out in state WaitingForInfo. Closing.

(I am just a bot, though. Please speak up if this is a mistake or you have the requested information.)