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: remove SizedReaderAt #15818

Closed
rsc opened this issue May 24, 2016 · 3 comments
Closed

io: remove SizedReaderAt #15818

rsc opened this issue May 24, 2016 · 3 comments

Comments

@rsc
Copy link
Contributor

rsc commented May 24, 2016

CL 21492 added this interface during the Go 1.7 cycle. The CL description says:

io: add ReadAtSizer interface

ReadAtSizer is a common abstraction for a stateless,
concurrently-readable fixed number of bytes.

This interface has existed in various codebases for over 3 years (previously
usually named SizeReaderAt). It is used inside Google in dl.google.com
(mentioned in https://talks.golang.org/2013/oscon-dl.slide) and other
packages. It is used in Camlistore, in Juju, in the Google API Go client, in
github.com/nightlyone/views, and 33 other pages of Github search results.

It is implemented by io.SectionReader, bytes.Reader, strings.Reader, etc.

Time to finally promote this interface to the standard library and give it a
standard name, blessing it as best practice.

(A later CL renamed it to SizedReaderAt.)

I tried in CL 23374 to make os.File implement this interface, since it already implements io.ReaderAt. But the natural Size method there needs to return an error: the underlying call to find the size may fail, and that failure needs to be reported. Returning 0 is incorrect, since the caller might conclude the file is empty, returning -1 may break various slice bounds preparing for a read, and returning a very large number could result in very large allocations. In this case, there needs to be some way to report an actual error.

This seems like a general problem: ReadAt can return an error, indicating that the underlying resource can go bad, but Size cannot. There's no obvious reason why Size fundamentally can't fail. Certainly if the file were accessed over the network, trying to find the Size could easily fail. Contrast this with ReadSeeker, which is often used for non-concurrent random access to data: the Seek(0, 2) operation reports the size of the data, but also an error.

This problem with the inability to report errors suggests that, contrary to the CL description, the SizedReaderAt interface may actually not be best practice.

I'm going to remove it for Go 1.7:
it's late in the Go 1.7 cycle, there are questions about the semantics, Brad is away, and anyone who really needs this interface can continue to define it themselves for another cycle. (Also the name is still awkward.)

@rsc rsc added this to the Go1.7Beta milestone May 24, 2016
@rsc rsc self-assigned this May 24, 2016
@nightlyone
Copy link
Contributor

Sorry, I strongly disagree here.

Interface io.SizedReaderAt was not intended for os.File, but for any use case in which the size is known ahead of time. If anything this could be made clearer in the docs.

Correct usage of io.SizedReaderAt with an os.File would be to wrap it in io.SectionReader with the results of a previous os.Stat-call. Then the ReadAt-Method will correctly report an error.

I could prepare a CL with a better example here.

NOTE: Undoing a days long discussion for a years long feature request in just 3h is a bit fast, isn't it?

@dominikh
Copy link
Member

NOTE: Undoing a days long discussion for a years long feature request in just 3h is a bit fast, isn't it?

New API can be added at any point in time. But once an API has been part of a release, it cannot be removed again. Since the next release is in feature freeze and past the point for long winded discussions, it is better to remove a questionable API again instead of being stuck with it. It can be re-evaluated in the next iteration.

@rsc
Copy link
Contributor Author

rsc commented May 24, 2016

To respond to the NOTE, what @dominikh is exactly right. We need to discuss this further, it would be best to do that when we're not under the crunch of getting a release out the door (that's a good way to make bad decisions), and in any event the people needed for the discussion (at the least, me and Brad) will not be available simultaneously again until September (Brad is away now and I will be away when he returns). There's basically no harm to delaying this one cycle.

Thanks for filing the new issue.

@golang golang locked and limited conversation to collaborators May 24, 2017
@rsc rsc removed their assignment Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants