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

Jump to bottom

io: TeeReader does not write out necessary fully #50491

Closed
mitar opened this issue Jan 7, 2022 · 4 comments
Closed

io: TeeReader does not write out necessary fully #50491

mitar opened this issue Jan 7, 2022 · 4 comments
Labels
FrozenDueToAge

Comments

@mitar
Copy link
Contributor

mitar commented Jan 7, 2022

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

$ go version
go version go1.17.6 linux/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=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/mitar/.cache/go-build"
GOENV="/home/mitar/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/mitar/.go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/mitar/.go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.17.6"
GCCGO="/usr/bin/gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/dev/null"
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 -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build2708945409=/tmp/go-build -gno-record-gcc-switches"

What did you do?

I was looking at the code of TeeReader's Read function and I saw:

func (t *teeReader) Read(p []byte) (n int, err error) {
	n, err = t.r.Read(p)
	if n > 0 {
		if n, err := t.w.Write(p[:n]); err != nil {
			return n, err
		}
	}
	return
}

What did you expect to see?

That writes will always succeed fully or an error would be returned.

What did you see instead?

  • That Write is called only once, so if Write writes less than n bytes, some bytes are lost and not written out.
  • If there is an error during Write the number of bytes written is returned to the reader, not the bytes read.

Possible solution

func (t *teeReader) Read(p []byte) (n int, err error) {
	n, err = t.r.Read(p)
	for written := 0; written < n; {
		m, err := t.w.Write(p[written:n])
		if err != nil {
			return n, err
		}
		written += m
	}
	return
}

Maybe some logic to detect too many empty write attempts should be added there (so if Write keeps returning 0 for m).
@seankhliao
Copy link
Member

From https://pkg.go.dev/io#Writer

Write must return a non-nil error if it returns n < len(p)

Write is not allowed to succeed if it's short

Closing as working as intended.

@mitar
Copy link
Contributor Author

mitar commented Jan 7, 2022

Interesting. I missed that. Thanks.

But the second point still holds: on error the number of written bytes is returned to the reader and not the number of read bytes. I think that should be fixed.

@seankhliao
Copy link
Member

Any error encountered while writing is reported as a read error.

So the number reported should match the write error

@mitar
Copy link
Contributor Author

mitar commented Jan 7, 2022 •
edited

I would disagree. If reader wants to know where to resume reading after a failure, this is not possible. I think it is good that the error is returned, but n should be that of the read.

@golang golang locked and limited conversation to collaborators Jan 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@mitar @gopherbot @seankhliao