proposal: bytes: make Reader.WriteTo side effect free when the Reader is empty

[Jorropo]

Motivations

I recently got bitten because some code was doing concurrent io.Copy from shared empty bytes.Reader.
This code was safe because the bytes.Reader.WriteTo function was hidden in a io.NopCloser, so io.Copy was using bytes.Reader.Read, however adding support of io.WriterTo to io.NopCloser had the side effect of no longer hidding bytes.Reader.WriteTo which trigger a race in io.Copy.

Underlying issue

The underlying issue is that bytes.Reader.Read and bytes.Reader.WriteTo have different side effects when called with an empty reader.

• bytes.Reader.WriteTo invalidate the previously returned rune when the reader is empty.
• bytes.Reader.Reader invalidate the previously returned rune only when the reader is not empty.

My understanding is that there is a weak contract about implementing io.WriterTo on objects also implementing io.Reader.
Is that implementation details aside (like zero copying), the side effects should be behaviorally equivalent between doing io.ReadAll(r) and r.WriteTo(io.Discard), and this weak contract is the reason io.Copy is allowed to attempt to use io.WriterTo when available.
However bytes.Reader doesn't do that.

Proposal

In bytes.Reader.WriteTo only invalidate the previously red rune if the buffer is not empty.
In code:

r := bytes.NewReader(data)
r.ReadRune() // r must now be empty
r.WriteTo(w)
r.UnreadRune() // Currently, err == errors.New("bytes.Reader.UnreadRune: previous operation was not ReadRune")
// This proposal err == nil and r.i = r.prevRune

Note that this behaviour is currently what we have with r.Read

r := bytes.NewReader(data)
r.ReadRune() // r must now be empty
r.Read(b)
r.UnreadRune() // Currently, err == nil and r.i = r.prevRune

Objective

This would harmonize side effects between bytes.Reader.Read and bytes.Reader.WriteTo when the Reader is empty and allows for concurrent bytes.Reader.WriteTo call with empty buffers.

[ianlancetaylor]

I'm not sure I understand all the details here.

However, in general, WriteTo is defined on a reader, and calling WriteTo tells the reader to write all available data to the writer passed to WriteTo. In general this can't be called concurrently. In particular, bytes.Reader.WriteTo can't be called concurrently in the general case.

I suppose that it does little harm to call WriteTo concurrently if the bytes.Reader is empty. But that is an odd special case to pick out.

It seems to me that the code that is making concurrent calls to WriteTo on a single bytes.Reader is broken, and that that code should be fixed. If this code only happens in a test, then the test should be fixed.

[Jorropo]

But that is an odd special case to pick out.

I agree, however that what bytes.Reader.Read already do, and we have tests and code in the std relies on this behaviour from Read, if compatilibity was not a concern I would be for the inverse patch (make bytes.Reader.Read concurrent-unsafe so -race would pick it up).

Here is what the patch would be (tests aside):

func (r *Reader) WriteTo(w io.Writer) (n int64, err error) {
-	r.prevRune = -1
	if r.i >= int64(len(r.s)) {
		return 0, nil
	}
+	r.prevRune = -1

To follow what Read do:

func (r *Reader) Read(b []byte) (n int, err error) {
	if r.i >= int64(len(r.s)) {
		return 0, io.EOF
	}
	r.prevRune = -1

don't think we can break Read's behaviour, but hopefully we can make WriteTo coherent.

[ianlancetaylor]

My opinion is WriteTo is already coherent, and if other parts of the standard library assume that it's OK to call io.Copy concurrently on an empty bytes.Reader, then we should fix those parts of the standard library.

[Jorropo]

if other parts of the standard library assume that it's OK to call io.Copy concurrently on an empty bytes.Reader, then we should fix those parts of the standard library

I agree with that statement.
@ianlancetaylor should this proposal be closed and open the reverse one (make bytes.Reader.Read invalidate r.prevRune on call so thoses cases would show up in -race) ?
Or would that be go2 since this would be a breaking change ?

[ianlancetaylor]

I think it would be OK to change bytes.Reader.Read but it doesn't seem very important.

[Jorropo]

Closing this then.

[bradfitz]

I think it would be OK to change bytes.Reader.Read but it doesn't seem very important.

{bytes,strings}.Reader already has a few special cases making things concurrently safe. Read is one of those.

[gopherbot]

Change https://go.dev/cl/401414 mentions this issue: bytes,strings: make Reader.WriteTo also safe for concurrent use