proposal: io: default ByteReader from Reader

[pierrec]

Background & Motivation

When using some APIs such as ReadUvarint, sometimes the need arise to provide an io.ByteReader when only an io.Reader is available. Currently, there are 2 main ways this is addressed:

• wrap the Reader with a new type which implements io.ByteReader
• use bufio.Reader that already implements io.ByteReader

The downsides of the current solutions are:

• multiple implementations of the same type
• instantiating a large object for a simple use

A quick look at the standard library shows that the second option is usually the preferred method:
compress/bzip2
compress/flate
compress/lzw
encoding/xml
image/gif

Goals

Simplify usage of APIs requiring io.ByteReader's.
Reduce memory footprint when needing an io.ByteReader from an io.Reader.

Design

Provide a new type that implements the wrapper. Something like below.

// byteReader turns an io.byteReader into an io.ByteReader.
type byteReader struct {
	io.Reader
}

// New returns an io.ByteReader from an io.byteReader.
// If r is already a ByteReader, it is returned.
func New(r io.Reader) io.ByteReader {
	if r, ok := r.(io.ByteReader); ok {
		return r
	}
	return &byteReader{r}
}

func (r *byteReader) ReadByte() (byte, error) {
	var buf [1]byte
	n, err := r.Reader.Read(buf[:])
	if err != nil {
		return 0, err
	}
	if n == 0 {
		return 0, io.ErrNoProgress
	}
	return buf[0], nil
}

To be decided

The package where this type would live: io, ioutil other?

Backwards Compatibility

This proposal is backward compatible.

[OneOfOne]

Reading one byte at a time without caching would be extremely slow for any real io, just something to consider.

[ianlancetaylor]

As you show, you can already do this yourself. So the only question is whether this should be in the standard library. Is this really something that comes up often enough to add to the standard library? https://golang.org/doc/faq#x_in_std

[pierrec]

@OneOfOne yes that is true, hence why it falls back to the underlying Reader implementation of ByteReader if it exists.

@ianlancetaylor indeed. And I filed this proposal after thinking about this and coming to the conclusion that this could go with the ioutil.NopCloser, which I do use from time to time as it is convenient albeit not too often. Such would be the case for the ByteReader.

Not sure how this comes up in practice outside of the standard library, I havent investigated this yet. I could if this helps.

[ianlancetaylor]

NopCloser was added to the standard library because external packages kept repeating that code. It made sense to consolidate a single version.

It's not clear to me that external package keep repeating the creation of an io.ByteReader. For most uses a bufio.Reader works well and is more efficient. The case where people want the less efficient, albeit smaller, io.ByteReader seems uncommon to me. But I might change my mind if there are many existing examples where programs have chosen to implement a small io.ByteReader.

[rsc]

The reason that io.ByteReader exists at all is that some code wants to read one byte at a time, and that really can't be done well without an underlying larger buffer. That is, Read of 1-byte slices is so suboptimal to be worth defining this added interface. This is why Uvarint takes an io.ByteReader in the first place instead of an io.Reader.

Having a "default" implementation that converts io.Reader to io.ByteReader by making 1-byte reads is going to have all the slowness we were avoiding by not doing that in the implementations.

In general having a ReadByte method is an assertion "I have an efficient way to read 1 byte at a time". This would not be making that claim accurately.

We could document this better in the io.ByteReader docs. The right answer for your case is to add a buffer, likely using bytes.Buffer, as high up in the call stack as possible, so that the buffer is amortized across as many calls as possible.

[pierrec]

@rsc thank you it makes sense.
Adding a note to the io.ByteReader interface would probably clarify its usage, something along the lines of:

// ByteReader is the interface that wraps the ReadByte method.
//
// ReadByte reads and returns the next byte from the input or
// any error encountered. If ReadByte returns an error, no input
// byte was consumed, and the returned byte value is undefined.
//
// When ReadByte is provided, it defines an efficient way of reading 
// bytes one at a time from an io.Reader, usually by using an underlying
// large buffer.
type ByteReader interface {
	ReadByte() (byte, error)
}

[gopherbot]

Change https://golang.org/cl/221380 mentions this issue: io: document that ReadByte is for efficiency

[ianlancetaylor]

As @robpike says on CL 221380:

This feels out of place, a programming lesson in the middle of a list of simple I/O interfaces.

It's a tutorial about when this might be useful, not documentation about what it does.
This is information for an example maybe, or a blog post, but doesn't belong here.

[rsc]

@robpike said he would take a stab at a CL for wording an appropriate comment.

[rsc]

Based on the discussion above, however, the proposal to add a helper wrapper that does 1-byte reads seems like a likely decline.

[gopherbot]

Change https://golang.org/cl/223097 mentions this issue: io: add a comment about how to turn a Reader into BytReader

[rsc]

No change in consensus, and Rob's CL landed, so declining.