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

Jump to bottom

embed: openFile should implement ReaderAt #57803

Closed
ncruces opened this issue Jan 15, 2023 · 15 comments
Closed

embed: openFile should implement ReaderAt #57803

ncruces opened this issue Jan 15, 2023 · 15 comments
Labels
Proposal Proposal-Accepted
Milestone
Go1.21

Comments

@ncruces
Copy link
Contributor

ncruces commented Jan 15, 2023

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.
@gopherbot gopherbot added this to the Proposal milestone Jan 15, 2023
@ncruces
Copy link
Contributor Author

ncruces commented Jan 15, 2023 •
edited

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
Copy link
Contributor

magical commented Jan 19, 2023

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
Copy link
Contributor Author

ncruces commented Jan 19, 2023

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
Copy link
Contributor

magical commented Jan 19, 2023

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
Copy link
Contributor

rsc commented Mar 15, 2023

This seems completely reasonable.

@ncruces
Copy link
Contributor Author

ncruces commented Mar 27, 2023

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

@ianlancetaylor
Copy link
Contributor

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
Copy link
Contributor

rsc commented Mar 29, 2023

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
Copy link

SGTM!

@rsc
Copy link
Contributor

rsc commented Apr 6, 2023

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

@ncruces ncruces mentioned this issue Apr 7, 2023
Closed
@gopherbot
Copy link

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

This was referenced Apr 12, 2023
Closed
Merged
@rsc
Copy link
Contributor

rsc commented Apr 12, 2023

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

@rsc rsc changed the title proposal: embed: openFile should implement ReaderAt embed: openFile should implement ReaderAt Apr 12, 2023
@rsc rsc modified the milestones: Proposal, Backlog Apr 12, 2023
@paralin paralin mentioned this issue Apr 13, 2023
Closed
@paralin
Copy link
Contributor

paralin commented Apr 13, 2023

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
Copy link
Member

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

@gopherbot
Copy link

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

gopherbot pushed a commit that referenced this issue May 31, 2023
@ianlancetaylor @gopherbot
doc/go1.21: mention that embedded files implement ReadAt
30f8d5b 
For #57803

Change-Id: I8e33f4dd3fc3071bfbf4d2848faefbe8488f5742
Reviewed-on: https://go-review.googlesource.com/c/go/+/499418
TryBot-Bypass: Ian Lance Taylor <iant@google.com>
Reviewed-by: Eli Bendersky <eliben@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
@dmitshur dmitshur removed this from the Backlog milestone Jun 4, 2023
@dmitshur dmitshur added this to the Go1.21 milestone Jun 4, 2023
Nasfame pushed a commit to golangFame/go that referenced this issue Jun 4, 2023
@ianlancetaylor @Nasfame
doc/go1.21: mention that embedded files implement ReadAt
018ed0f 
For golang#57803

Change-Id: I8e33f4dd3fc3071bfbf4d2848faefbe8488f5742
Reviewed-on: https://go-review.googlesource.com/c/go/+/499418
TryBot-Bypass: Ian Lance Taylor <iant@google.com>
Reviewed-by: Eli Bendersky <eliben@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Proposal Proposal-Accepted
Projects
None yet
Milestone
Go1.21
Development

Successfully merging a pull request may close this issue.

embed: implement openFile.ReadAt
9 participants
@rsc @magical @paralin @cpuguy83 @dmitshur @ianlancetaylor @ncruces @gopherbot @seankhliao