Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
welkeyever opened this issue Jan 12, 2022 · 4 comments
Closed
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.

Comments

@welkeyever
Copy link

welkeyever commented Jan 12, 2022

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
}
@seankhliao seankhliao changed the title affected/package: bufio bufio: Reader.Peek should return EOF when there's nothing more to read Jan 12, 2022
@seankhliao seankhliao added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jan 12, 2022
@ianlancetaylor
Copy link
Contributor

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.

@ianlancetaylor ianlancetaylor added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Jan 12, 2022
@welkeyever
Copy link
Author

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
Copy link
Contributor

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.

@seankhliao seankhliao added WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. and removed WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. labels Jun 12, 2022
@gopherbot
Copy link

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.)

@golang golang locked and limited conversation to collaborators Jul 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Projects
None yet
Development

No branches or pull requests

4 participants