bufio: Writer.ReadFrom doesn't always buffer

[eikenb]

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]

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.

[eikenb]

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().

[gopherbot]

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