bufio: return error from UnreadByte/UnreadRune after Peek

[mjgarton]

Please answer these questions before submitting your issue. Thanks!

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

go version go1.7.3 linux/amd64

What operating system and processor architecture are you using (go env)?

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/mgarton/gopath"
GORACE=""
GOROOT="/home/mgarton/go"
GOTOOLDIR="/home/mgarton/go/pkg/tool/linux_amd64"
CC="gcc"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build323320002=/tmp/go-build -gno-record-gcc-switches"
CXX="g++"
CGO_ENABLED="1"

What did you do?

I ran this:

func TestPeekAndUnread(t *testing.T) {

	message := []byte("hello world. how are you?")
	br := bufio.NewReaderSize(bytes.NewReader(message), 16)

	for i := 0; i < 16; i++ {
		_, _, err := br.ReadRune()
		if err != nil {
			t.Fatal(err)
		}
	}

	_, err := br.Peek(1)
	if err != nil {
		t.Fatal(err)
	}

	err = br.UnreadRune()
	if err != nil {
		t.Error(err)
	}
}

What did you expect to see?

Either the test should pass, or the documentation for bufio.Reader.UnreadRune should mention that Peek can prevent UnreadRune from working.

What did you see instead?

The test fails.

If someone tells me whether this is a code bug or a documentation issue, I'm happy to attempt a CL

[mjgarton]

I suspect the code is working as intended. Do people agree that the documentation for UnreadRune() could be clearer? It wasn't clear to me that Peek() is considered a "read operation" in this context. Or perhaps peek isn't considered a "read operation" and the documentation should say "read or peek operation" or similar?

[titanous]

This is working as intended. Peek is definitely a read operation, though it does not advance the reader. Note the two mentions of "read" in the method documentation:

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.

It may make sense to slightly clarify the documentation about what Peek does to make it even more clear that a read is being performed.

[mjgarton]

Another observation is that typically Peek doesn't stop UnreadRune working, it only does so under certain conditions (such as those in my attached test) So even adding something saying that Peek is a read operation doesn't clarify things entirely.

[kisielk]

It seems like a bug and/or incorrect documentation to me. If the range of the for loop is changed to 15, the test passes. That's because there's still enough data in the buffer to satisfy the peek and internally fill is not called and the internal read index is not updated.

Either way something is wrong: If Peek is not considered a read operation, then UnreadRune shouldn't return an error in either case. If Peek is a read operation then UnreadRune should also return an error in that case. I suspect given the implementation it only makes sense to consider Peek a read since it may change the underlying read buffer and make unreading impossible.

[keithrel]

This is not restricted to ReadRune/UnreadRune - ReadByte/UnreadByte have the same error
for essentially the same reason. Its the pathological case of a Peek() right after reading the last byte/rune which triggers the call to fill().

[ALTree]

If Peek is a read operation then UnreadRune should also return an error in that case.

I suspect this is the case. So this is not just a doc issue, because we also need to change UnreadRune so that is always returns an error after a Peek. Moving to go1.10 since fixing this will maybe require code changes and we're deep in the freeze.

[rsc]

UnreadRune and UnreadByte were defined before Peek existed. I agree that if Peek has happened since the last rune or byte read, these should return an error.

[rsc]

If anyone wants to send a CL, please do (for Go 1.10).

[mjgarton]

I will have a go at this in the next day or two.

[gopherbot]

CL https://golang.org/cl/46850 mentions this issue.

[heschi]

I think some kind of documentation change is still needed. The commit message says:

Change Peek to invalidate the unread buffers in all cases (as allowed according to the documentation) and thus prevent hiding bugs in the caller.

What exactly in the documentation says that this behavior is allowed? The documentation for Unread says

UnreadByte unreads the last byte. Only the most recently read byte can be unread.

and nothing in Peek says anything about Unread*. IMO the Unread* operations should document more specifically which calls can precede them. For another example, the documentation doesn't say whether I can Discard() and then Unread() ... though I can't imagine why anyone would do that.

[rsc]

Thanks for pointing out that this change went in @heschik. I rolled it back. This was supposed to be for Go 1.10.

[dsnet]

Do we still want to make this change? It's after the freeze... or are we going to push to Go1.11?

[bradfitz]

@dsnet, let's do this for Go 1.10. You want to re-apply it and include some docs?

[rsc]

Too late now, push to Go 1.11

[mjgarton]

How about for 1.12? Not having this has made real bugs hard to find twice for me, so I still think it's worth going.

[ianlancetaylor]

@mjgarton Want to send a patch? Thanks.

[gopherbot]

Change https://golang.org/cl/149297 mentions this issue: bufio: make Reader.Peek invalidate Unreads

[bcmills]

Here is a concrete call site that will silently break as a result of this change, because it does not check the error from UnreadByte (see also #20803 etc.):
https://github.com/Comcast/gots/blob/06ffb31a36ca3348e4e20a653c07d85de7ff9123/packet/io.go#L51-L59

(Note that rp may alias r if r.Size() >= PacketSize.)

It is not obvious to me whether that call site is already silently broken in some cases.

[bcmills]

The call site in github.com/Comcast/gots is definitely buggy, but it's not obvious to me that the change to bufio is correct either.

In particular, if we execute b.Peek(b.Size()-1), then we should be able to refill the buffer enough to finish the Peek while retaining a spare byte of capacity for the potential UnreadByte, without the need to add a special caveat about not executing a Peek in between a Read and the corresponding UnreadByte.

[bcmills]

For that matter, it's not obvious that we need to invalidate UnreadByte for any Peek: the fact that we implement UnreadByte by rewinding the buffer is an implementation detail, not an intrinsic property of the API. (For example, we could set r = -1 to mean “tack lastByte on to the beginning of the data”.)

Preserving UnreadByte might complicate Read somewhat, but would avoid an awkward semantic change.