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.

Sign up for GitHub

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

bufio: Reader cannot buf content greater than 32KB #33264

Closed
TennyZhuang opened this issue Jul 24, 2019 · 7 comments
Closed

bufio: Reader cannot buf content greater than 32KB #33264

TennyZhuang opened this issue Jul 24, 2019 · 7 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.

Comments

@TennyZhuang
Copy link
Contributor

TennyZhuang commented Jul 24, 2019 •
edited

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

$ go version
1.12.7

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

GOARCH="amd64"

GOBIN=""

GOCACHE="/Users/zhuangtianyi/Library/Caches/go-build"

GOEXE=""

GOFLAGS=""

GOHOSTARCH="amd64"

GOHOSTOS="darwin"

GOOS="darwin"

GOPATH="/Users/zhuangtianyi/go"

GOPROXY=""

GORACE=""

GOROOT="/usr/local/Cellar/go/1.12.7/libexec"

GOTMPDIR=""

GOTOOLDIR="/usr/local/Cellar/go/1.12.7/libexec/pkg/tool/darwin_amd64"

GCCGO="gccgo"

CC="clang"

CXX="clang++"

CGO_ENABLED="1"

GOMOD="/Users/zhuangtianyi/go/src/github.com/TennyZhuang/net/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/_6/hnkq5pbs6l3d94ngc525pnnh0000gp/T/go-build099861045=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

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

Run the server, then just curl it.

What did you expect to see?

The buffer should work, then the output should be:

2019/07/24 20:28:08 Read 4194304 bytes
2019/07/24 20:28:09 Read 4194304 bytes
2019/07/24 20:28:10 Read 4194304 bytes
2019/07/24 20:28:11 Read 4194304 bytes
2019/07/24 20:28:12 Read 4194304 bytes
2019/07/24 20:28:14 Read 4194304 bytes
...

What did you see instead?

2019/07/24 20:30:15 Read 32768 bytes
2019/07/24 20:30:16 Read 32768 bytes
2019/07/24 20:30:17 Read 32768 bytes
2019/07/24 20:30:18 Read 32768 bytes
2019/07/24 20:30:19 Read 32768 bytes
2019/07/24 20:30:20 Read 32768 bytes
2019/07/24 20:30:21 Read 32768 bytes
2019/07/24 20:30:22 Read 32768 bytes
2019/07/24 20:30:23 Read 32768 bytes
2019/07/24 20:30:24 Read 32768 bytes
2019/07/24 20:30:25 Read 32768 bytes
2019/07/24 20:30:26 Read 32768 bytes
2019/07/24 20:30:27 Read 32768 bytes
2019/07/24 20:30:28 Read 32768 bytes
...

Investigation

To reduce the copy buffer, io.Copy will follow the behavior of WriterTo and ReaderFrom firstly.

bufio.Reader implements WriterTo, which will call the w's ReadFrom if possible.

And the implementation of http.ResponseWriter is http.(*response), it implements ReaderFrom to optimize for sendfile. But it has a bad fallback behavior on non-file reader, it call io.CopyBuffer with a small buffer(32KB), which break the buffer behavior of bufio.Reader.

All of above caused the unexpected behavior that bufio.Reader really can not buf anything more than 32KB.

A workaround is just wrap a nothing reader wrapped on *bufio.Reader, but it's not elegant.
@julieqiu julieqiu changed the title bufio.Reader not worked with http.ResponseWriter bufio: Reader not worked with http.ResponseWriter Jul 29, 2019
@julieqiu julieqiu changed the title bufio: Reader not worked with http.ResponseWriter bufio: Reader cannot buf content greater than 32KB Jul 29, 2019
@julieqiu
Copy link
Member

/cc @ianlancetaylor

I was able to reproduce the described behavior by running the code in https://play.golang.org/p/i4IqIQPMqFd locally.

@julieqiu julieqiu added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jul 29, 2019
@randall77
Copy link
Contributor

We could check in (*response).ReadFrom before we allocate the buffer, if the source is a buffered reader itself, and use WriteTo instead. It's not ideal, but would work.
The real fix is to introduce a buffer size interface and in io.Copy when both src and dst implement WriteTo and ReadFrom, respectively, pick the one with the larger buffer. bufio.Reader has a Size method, maybe that's the interface. This would change the spec of io.Copy subtly, which is probably not allowed. Currently it is spec'd to always chose WriterTo in preference to ReaderFrom.

@ianlancetaylor
Copy link
Contributor

This is likely a dup of #16474.

@TennyZhuang
Copy link
Contributor Author

TennyZhuang commented Mar 12, 2020 •
edited

@randall77 I meet this bug again and it's difficult and confusing to locate this bug, is it ok to fix it use the workaround? I'd like to submit a change about it.

@randall77
Copy link
Contributor

Which workaround do you mean? There are a few in this thread and in #16474.

@smithyj
Copy link

smithyj commented Jul 13, 2020

you can use bufio.NewReaderSize and bufio.NewWriterSize method

@davecheney
Copy link
Contributor

Closing as duplicate of #16474

@mailsmail mailsmail mentioned this issue Feb 9, 2021
Closed
@golang golang locked and limited conversation to collaborators Jul 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
7 participants
@davecheney @ianlancetaylor @julieqiu @randall77 @gopherbot @TennyZhuang @smithyj