NewReadOnly fails on exhausted ReadSeeker

[ribasushi]

• An os.File has GenerateIndex() called on
• This puts the os.File's underlying Reader at EOF
• If one then calls NewReadOnly the check here falls through to the io.Reader, sees the EOF and aborts

Observe the workaround at lines 81~83 in the sequence here. A potential fix is to rewind when the argument is a ReadSeeker, before dropping to the Reader part.

[rvagg]

yeah, I think you're right, because the non-Reader case specifically offsets to 0, so we have a fork in behaviour if you give it a full Reader.. I suppose a case to check for a Seeker is in order, although I'm not really sure why the check for Reader even exists. Perhaps a potential optimisation? I wonder if we shouldn't just remove that and pass everything through the internalio.NewOffsetReadSeeker.

@masih @willscott any memories of why we might be special-casing a Reader here when we have a ReaderAt in our hands?

[willscott]

the unwrapping / wrapping is mostly there for handling streaming use cases - if we're getting a car inbound as a reader over the network we want to be wrapping/unwrapping it such that we are pulling from the underlying buffer as we get the data.

[masih]

+1 for consistency in behaviour; thank you for pointing this out.

[rvagg]

I'm not sure I fully understand this, I wouldn't expect ReaderAt and ReadSeekerAt to be interfaces that you'd get if you were streaming a CAR. I'm probably missing something about the scope of "streaming" APIs in Go.

In terms of next steps, we could add a check for a Seeker and do a seek on that, but the confusing bit for me is that we're requiring a ReaderAt, coming in via NewReadOnly(). Are we really going to have a streaming CAR come in that's both a ReaderAt and a Reader? Why can't we just ditch the check for Reader entirely and use NewOffsetReadSeeker for all cases on the ReaderAt?