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

bytes: Buffer.ReadFrom should not ignore io.EOF #66155

Closed
blackbeans opened this issue Mar 7, 2024 · 4 comments
Closed

bytes: Buffer.ReadFrom should not ignore io.EOF #66155

blackbeans opened this issue Mar 7, 2024 · 4 comments

Comments

@blackbeans
Copy link

Go version

go version go1.22.0 darwin/arm64

Output of go env in your module/workspace:

GO111MODULE='on'
GOARCH='arm64'
GOBIN=''
GOCACHE='/Users/xx/Library/Caches/go-build'
GOENV='/Users/xxx/Library/Application Support/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=' -mod=readonly'
GOHOSTARCH='arm64'
GOHOSTOS='darwin'
GOINSECURE=''
GOMODCACHE='/Users/xxxx/go/pkg/mod'
GONOPROXY='git.xxx.com'
GONOSUMDB='git.xxx.com'
GOOS='darwin'
GOPATH='/Users/xxxxx/go'
GOPRIVATE='git.xxxx.com'
GOPROXY='https://goproxy.cn,direct'
GOROOT='/usr/local/go'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/usr/local/go/pkg/tool/darwin_arm64'
GOVCS=''
GOVERSION='go1.22.0'
GCCGO='gccgo'
AR='ar'
CC='clang'
CXX='clang++'
CGO_ENABLED='1'
GOMOD='/Users/xxxx/workspace/xxxx/opensource/xxxx/go.mod'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -arch arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -ffile-prefix-map=/var/folders/6y/r10dlmq565d6y6jyy6w12xp80000gn/T/go-build2825296061=/tmp/go-build -gno-record-gcc-switches -fno-common'

What did you do?

	
		in, _ := os.Open("../resources/demo.flac")
		defer in.Close()

		buff := bytes.NewBuffer(make([]byte, 0, MB))
		//read by parttition
		for i := 0; i < 1000; i++ {
			buff.Reset()
			length, err := buff.ReadFrom(io.LimitReader(in, MB))
			if nil != err && !errors.Is(err, io.EOF) {
				t.FailNow()
			} else if errors.Is(err, io.EOF) {
                               // this does not happen
				break
			}
			t.Logf("read:%d\n", length)
		}

What did you see happen?

This code block breaks util loop 1000 times ,but we expect this loop break when the readFrom throws io.EOF .

The bytes library shows the readFrom source code as below :

// ReadFrom reads data from r until EOF and appends it to the buffer, growing
// the buffer as needed. The return value n is the number of bytes read. Any
// error except io.EOF encountered during the read is also returned. If the
// buffer becomes too large, ReadFrom will panic with [ErrTooLarge].
func (b *Buffer) ReadFrom(r io.Reader) (n int64, err error) {
	b.lastRead = opInvalid
	for {
		i := b.grow(MinRead)
		b.buf = b.buf[:i]
		m, e := r.Read(b.buf[i:cap(b.buf)])
		if m < 0 {
			panic(errNegativeRead)
		}

		b.buf = b.buf[:i+m]
		n += int64(m)
		if e == io.EOF {
			return n, nil // e is EOF, so return nil explicitly
		}
		if e != nil {
			return n, e
		}
	}
}

The source code throws nil insteadOf io.EOF .

What did you expect to see?

I Suggest readFrom throws io.EOF,so that keep the same as others io operatoions.

@Jorropo
Copy link
Member

Jorropo commented Mar 7, 2024

This behavior is explicitly documented by io.ReaderFrom:

ReadFrom reads data from r until EOF or error. The return value n is the number of bytes read. Any error except EOF encountered during the read is also returned.

I don't see why it should return io.EOF, what would that mean ? io.ReadFrom read everything there is to read (so everything until io.EOF) unless an error happen.
As far as I can tell that means io.ReadFrom would never return a nil, error it would always return io.EOF or some other error.

@panjf2000
Copy link
Member

In Go, returning a non-nil error from a function/method indicates there is something wrong. In this case, EOF shouldn't be interpreted as a wrongness, it only manifests that there is no more data that can be read from a data source. Therefore, it makes sense not to return this error. You may find this behavior rational after checking out read(2) which shares the same philosophy (not treating EOF as an error) with io.ReaderFrom.

@panjf2000 panjf2000 changed the title import/path: bytes.Buffer readFrom method should not ignore io.EOF bytes: Buffer.ReadFrom should not ignore io.EOF Mar 7, 2024
@thediveo
Copy link

thediveo commented Mar 7, 2024

This would break a lot of existing code that has been programed to the documented contract.

@ianlancetaylor
Copy link
Contributor

Thanks, but we aren't going to make this change.

@ianlancetaylor ianlancetaylor closed this as not planned Won't fix, can't repro, duplicate, stale Mar 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants