embed: openFile should implement ReaderAt

[ncruces]

embed.openFile could trivially implement io.ReaderAt.

go/src/embed/embed.go

Lines 348 to 381 in 15605ca

	var (
	_ io.Seeker = (*openFile)(nil)
	)
	func (f *openFile) Close() error { return nil }
	func (f *openFile) Stat() (fs.FileInfo, error) { return f.f, nil }
	func (f *openFile) Read(b []byte) (int, error) {
	if f.offset >= int64(len(f.f.data)) {
	return 0, io.EOF
	}
	if f.offset < 0 {
	return 0, &fs.PathError{Op: "read", Path: f.f.name, Err: fs.ErrInvalid}
	}
	n := copy(b, f.f.data[f.offset:])
	f.offset += int64(n)
	return n, nil
	}
	func (f *openFile) Seek(offset int64, whence int) (int64, error) {
	switch whence {
	case 0:
	// offset += 0
	case 1:
	offset += f.offset
	case 2:
	offset += int64(len(f.f.data))
	}
	if offset < 0 || offset > int64(len(f.f.data)) {
	return 0, &fs.PathError{Op: "seek", Path: f.f.name, Err: fs.ErrInvalid}
	}
	f.offset = offset
	return offset, nil
	}

That's the entire proposal.

Rationale

io.ReaderAt can be emulated with io.ReadSeeker, but this doesn't satisfy the goroutine safety of io.ReaderAt: “Clients of ReadAt can execute parallel ReadAt calls on the same input source.”

This came up twice now on implementing wazero syscalls, that follow pread semantics: tetratelabs/wazero#967, and now tetratelabs/wazero#1037.
It got its own mention in that project's design rationale.

It is also of note that io.ReaderAt is directly called out on the fs.File documentation: A file may implement io.ReaderAt or io.Seeker as optimizations.

[ncruces]

If the proposal is accepted, I can happily do the legwork on implementation and testing. Or delegate it to the Go team, whichever is more efficient (the burden of reviewing untrusted code is often higher than the implementation, but I've contributed a couple fixes before, so I'm already familiar with the process).

[magical]

My 2¢: We shouldn't promise that files from embed.FS implement ReaderAt - we should leave the door open for transparent compression of embedded files (for example) in future versions of Go and in alternative toolchains. But providing it as an optional interface upgrade, when available, seems reasonable to me.

[ncruces]

I'm fine with that, but we already promise to implement io.Seeker.

For the embed case, I struggle to see a case where is Seek is “easy” but ReadAt “hard” (even considering e.g. compression, etc).

[magical]

I'm fine with that, but we already promise to implement io.Seeker.

You're right, I missed that detail. Ah well. Probably not worth worrying about then.

[rsc]

This seems completely reasonable.

[ncruces]

Should I do this, @rsc?
Is it likely to be accepted?

[ianlancetaylor]

This has not been accepted yet, but it is likely to be accepted. If you want to work on it, go ahead and mail a patch, but it won't be committed until the proposal is formally reviewed and accepted. Thanks.

[rsc]

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

[cpuguy83]

SGTM!

[rsc]

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

[gopherbot]

Change https://go.dev/cl/483235 mentions this issue: embed: implement openFile.ReadAt

[rsc]

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

[paralin]

Would it be possible to backport https://go-review.googlesource.com/c/go/+/483235 to the next go 1.20 release, or do we need to wait for go1.21? Thanks!

[seankhliao]

this wouldn't meet the bar for backporting (major bugs with no workaround)

[gopherbot]

Change https://go.dev/cl/499418 mentions this issue: doc/go1.21: mention that embedded files implement ReadAt