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: Writer.ReadFrom doesn't always buffer #23289

Closed
eikenb opened this issue Dec 30, 2017 · 3 comments
Closed

bufio: Writer.ReadFrom doesn't always buffer #23289

eikenb opened this issue Dec 30, 2017 · 3 comments
Labels
Documentation FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Unplanned

Comments

@eikenb
Copy link

eikenb commented Dec 30, 2017

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

1.9.2 (and tip)

Does this issue reproduce with the latest release?

yes

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

linux/amd64 (effects all)

What did you do?

Used bufio.Writer() to buffer writes.

What did you expect to see?

Buffered writes.

What did you see instead?

Writes weren't buffered if Writer wrapped by bufio.Writer implements ReadFrom.

Details

This came up with the SFTP library [1] which provides a client side remote file abstraction for working with the remote SFTP server. One of our users reported a bug [2] when their clients write throughput dropped by 5X. They had wrapped this file object using bufio as they had lots of small writes they wanted buffered up and sent in batch. When we added ReadFrom() to the file abstraction suddenly the buffering stopped and their throughput tanked.

I can see the point of passing the writes directly through as it is avoids an additional copy and is a nice optimization. But the point of using bufio is to buffer, so it seems like it might be a mistake to not buffer it in this case. If it is intended behavior, then I'd think it should be highlighted in the documentation as it sort of surprises you when you run into it.

I have uploaded a simple demonstration to https://play.golang.org/p/zXFtdo8oyAY

Thanks.

[1] https://github.com/pkg/sftp
[2] pkg/sftp#125
@dsnet
Copy link
Member

dsnet commented Dec 31, 2017

This is related to #16474 which notices similar issues. Fundamentally, the io.ReaderFrom and io.WriterTo is ill-specified as to how efficient it is supposed to be. It is impossible for a wrapper (like bufio.Writer) to know whether it's own implementation would have been more efficient or whether the underlying implementation would have been better.

The implementation of bytes.Buffer.ReadFrom is actually pretty efficient, except for the case where the Buffer is new (which is arguably common).

It seems that the best course is to just document this.

@dsnet dsnet changed the title Bufio.ReadFrom() doesn't always buffer bufio: Writer.ReadFrom() doesn't always buffer Dec 31, 2017
@dsnet dsnet changed the title bufio: Writer.ReadFrom() doesn't always buffer bufio: Writer.ReadFrom doesn't always buffer Dec 31, 2017
@eikenb
Copy link
Author

eikenb commented Jan 11, 2018

The workaround for this is to wrap the object that implements the slower ReadFrom is to hide the ReadFrom with a wrapping struct. Eg.

var f *slowReadFromObj := ...
type writerOnly struct { io.Writer }
bw := bufio.NewWriter(writerOnly{f})

The writerOnly struct with the single io.Writer interface field will hide everything on the wrapped object except the Write().

@ianlancetaylor ianlancetaylor added Documentation NeedsFix The path to resolution is known, but the work has not been done. labels Mar 28, 2018
@gopherbot
Copy link

Change https://golang.org/cl/103238 mentions this issue: bufio: document ReadFrom/WriteTo calls to underlying methods

@ianlancetaylor ianlancetaylor added this to the Unplanned milestone Mar 28, 2018
@golang golang locked and limited conversation to collaborators Mar 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
Documentation FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
4 participants
@eikenb @ianlancetaylor @dsnet @gopherbot