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

io: clearly document when CopyBuffer does not use the buffer #32306

Closed
bcmills opened this issue May 29, 2019 · 6 comments
Closed

io: clearly document when CopyBuffer does not use the buffer #32306

bcmills opened this issue May 29, 2019 · 6 comments
Labels
Documentation FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@bcmills
Copy link
Contributor

bcmills commented May 29, 2019

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)
@gopherbot gopherbot added this to the Proposal milestone May 29, 2019
@bcmills bcmills added v2 An incompatible library change and removed Proposal labels May 29, 2019
@bcmills bcmills modified the milestones: Proposal, Go2 May 29, 2019
@rsc
Copy link
Contributor

rsc commented May 29, 2019

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
Copy link
Contributor

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
Copy link
Contributor

@bcmills Any thoughts on #32306 (comment)?

@bcmills
Copy link
Contributor Author

bcmills commented Sep 3, 2019

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
Copy link
Contributor

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

@ianlancetaylor ianlancetaylor changed the title proposal: io: deprecate CopyBuffer io: clearly document when CopyBuffer does not use the buffer Sep 17, 2019
@ianlancetaylor ianlancetaylor added Documentation help wanted NeedsFix The path to resolution is known, but the work has not been done. and removed v2 An incompatible library change Proposal labels Sep 17, 2019
@ianlancetaylor ianlancetaylor modified the milestones: Go2, Unplanned Sep 17, 2019
@odeke-em
Copy link
Member

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.

@golang golang locked and limited conversation to collaborators Oct 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Documentation FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

5 participants