bufio: document initialization requirements/usability of zero value

[aka-rider]

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

go version go1.13.8 darwin/amd64

Does this issue reproduce with the latest release?

Yes

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

go env Output

$ go env
GO111MODULE="on"
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/rider/Library/Caches/go-build"
GOENV="/Users/rider/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GONOPROXY="gitlab.com/aigent/*"
GONOSUMDB="gitlab.com/aigent/*"
GOOS="darwin"
GOPATH="/Users/rider/wrk/go"
GOPRIVATE="gitlab.com/aigent/*"
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/Cellar/go/1.13.8/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.13.8/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/rider/wrk/go/src/github.com/aka-rider/nanoq/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/l8/_c7mlptn4zgbjb_xywymh2lm0000gn/T/go-build933390803=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

• create bufio.Reader
• call Reset
• try to read

https://play.golang.org/p/eCVyld7ImE3

What did you expect to see?

bufio.Reader does not panic on attempt to read

What did you see instead?

bufio.Reader panics on any attempt to read (fill the buffer from the underlying reader)

panic: bufio: tried to fill full buffer

goroutine 1 [running]:
bufio.(*Reader).fill(0x41a764, 0x4404d0)
	/usr/local/go/src/bufio/bufio.go:95 +0x2c0
bufio.(*Reader).ReadByte(0x41a764, 0xae9e0, 0x43e0a0, 0x9, 0x40c0c0, 0x432000)
	/usr/local/go/src/bufio/bufio.go:252 +0x40
main.main()
	/tmp/sandbox711084966/prog.go:12 +0x160

[aka-rider]

It is weird, as bufio.Reader.Reset uses the same code path as a bufio.NewReader, yet NewReader returns a working buffer and Reset leaves a reader unusable.

Debugger shows that b.buf is nil, hence:

func (b *Reader) fill() {
	...

	if b.w >= len(b.buf) {
		panic("bufio: tried to fill full buffer")
	}

b.w - write pos is 0 as it should, but len(b.buf) is also 0

[JOT85]

What's happening, is new(bufio.Reader) initialises (to 0 values) the bufio.Reader with buf as nil - so no space in the buffer (whereas bufio.NewReader initialises it with a default buffer size).
This means that trying to fill will fail since there is no space in the buffer.
Perhaps a more useful error message would be bufio: tried to fill 0 length buffer (which would be given if len(r.buf) == 0)?

[aka-rider]

Hi @JOT85
Ah, yes. You're right.

I think what would help is a function that constructs bufio.Reader with the non-empty buffer but without an underlying reader.
Specifically, this is useful when bufio.Reader is used together with sync.Pool.

It is possible to call bufio.NewReader(nil), but IMHO this code relies on the implementation details

[aka-rider]

On a second thought, we may legitimize bufio.NewReader(nil) in the documentation, saying that it is okay to create a dummy Reader assuming the upcoming Reset call, and additionally explicitly state that any read attempt on such buffer will cause a panic.

[ianlancetaylor]

Why not just call bufio.NewReader with a dummy io.Reader, such as bytes.NewReader(nil)? I'm not sure we want to support bufio.NewReader(nil). And I'm not sure we want to support new(bufio.Reader).

[bcmills]

At the very least, the documentation for bufio.Reader should mention something about its zero-value. Given that we strive for meaningful zero-values for other types, it does not seem unreasonable for users to expect the zero bufio.Reader to also be usable.

For example, (*Reader).Reset could initialize b.buf to a buffer of default size if it has not already been initialized. (The branch would likely be very predictable, so the only significant cost would be a few bytes of program text and an extra entry in the branch predictor.)

[aka-rider]

Hi @ianlancetaylor

Why not just call bufio.NewReader with a dummy io.Reader, such as bytes.NewReader(nil)?

I'm happy with either solution. My only concern is that the next person would repeat the same mistake again.

[aka-rider]

Hi @bcmills
I totally agree.

The branch would likely be very predictable, so the only significant cost would be a few bytes of program text and an extra entry in the branch predictor

I want to add that Reset is unlikely to be called on a performance critical path.

And just saying out loud that bufio.Writer has the same API.

[gopherbot]

Change https://go.dev/cl/633315 mentions this issue: bufio: document Reader must be created with New functions

[griesemer]

The panic was fixed with the fix for #45374.
It may still make sense to update the documentation. Leaving open for that.

[seankhliao]

since it works now, there doesn't seem to be anything left to document?

[griesemer]

It works if the reader is properly initialized, or if the zero value is reset with a non-nil io.Reader (which makes sense).

[mknyszek]

(Added back NeedsDecision because one of the labels that begins with Needs must be put on the issue to get it out of the gardening queue. If that's wrong, please feel free to put either NeedsFix or NeedsInvestigation. Thanks.)