io: clearly document when CopyBuffer does not use the buffer

[bcmills]

io.CopyBuffer is difficult to use appropriately: it has a subtle interaction with ReadFrom and WriteTo methods (#16474, #32276) that can result in unintended use of small buffers and/or unintended allocation of large slices.

For the cases that avoid that subtle interaction, io.CopyBuffer mostly duplicates the functionality of bufio.Reader, particularly given the (*bufio.Reader).Reset method (which allows a single bufio.Reader to wrap multiple io.Reader instances).

I propose that we deprecate io.CopyBuffer, and encourage explicit use of bufio.Reader as a replacement.

In particular, note that if the caller intends to always use the buffer, call sites of the form:

var buf = make([]byte, bufLen)

[…]

n, err = io.CopyBuffer(dst, src, buf)

can be expressed more clearly (and more robustly) as

var srcBuf = bufio.NewReaderSize(nil, bufLen)

[…]

srcBuf.Reset(src)
n, err = io.Copy(dst, srcBuf)

The two cases I've found where the existing behavior of io.CopyBuffer is always beneficial are:

• when the src argument is a *bytes.Reader (or similar io.WriterTo with an internal []byte containing its complete output), or
• when the src argument is a *strings.Reader (or similar io.WriterTo with an internal string containing the complete output) and the dst argument implements io.StringWriter.

In those cases, io.CopyBuffer avoids an extra copy without inducing an extra allocation. However, as noted in #16474 (comment), detecting those cases requires much more detailed information than what io.CopyBuffer currently uses (and what ReaderFrom and WriterTo currently provide).

Moreover, if the caller knows that the WriterTo method, if any, is likely to be more efficient than buffering, it's easy enough to invoke explicitly, with explicit use of buffering on the fallback path:

var srcBuf = bufio.NewReaderSize(nil, bufLen)

wt, ok := src.(io.WriterTo)
if !ok {
	srcBuf.Reset(src)
	wt = srcBuf
}
n, err = wt.WriteTo(dst)

[rsc]

CopyBuffer is meant to be used when profiling shows it would be helpful. I don't see any point to deprecating it in favor of more complex code using the much more general bufio.

[ianlancetaylor]

Perhaps the right fix here is to move the explicit tests for WriterTo and ReaderFrom from the shared implementation of Copy and CopyBuffer to only test for them in Copy. Then if someone explicitly calls CopyBuffer, it will always use the buffer they passed in.

[ianlancetaylor]

@bcmills Any thoughts on #32306 (comment)?

[bcmills]

I don't think that changing the behavior of CopyBuffer would comply with the principles laid out in the Go 2 transition document.

In particular, changing CopyBuffer to skip the WriterTo and ReaderFrom checks may cause a regression for users in the two cases where CopyBuffer really is beneficial, or for users who were accidentally relying on the specific number of resulting Read or Write calls (for example, due to an implicit assumption on Write atomicity or packet fragmentation).

[ianlancetaylor]

Our decision is that we aren't going to remove CopyBuffer. Instead, we should more clearly document how it behaves in the presence of WriterTo and ReaderFrom methods (it is already documented but only implicitly). It would also be good to provide an example showing how people can bypass type methods by embedding an io.Reader or io.Writer.

-- for @golang/proposal-review

[odeke-em]

Thank you @bcmills for this issue and @rsc and @ianlancetaylor for the discussion and direction!

@bcmills given the direction of this issue, this issue boils down to just documenting that if src implements io.ReaderFrom and dst implements io.WriterTo, then buf won't be used, and that then means that this issue is a duplicate of #32276 for which there is already a CL that I am going to review shortly. I shall therefore close this issue as a duplicate. Please feel free to reopen though.