Skip to content

Fix fdPread: shouldn't move file offset #967

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

Merged
merged 3 commits into from
Dec 29, 2022
Merged

Fix fdPread: shouldn't move file offset #967

merged 3 commits into from
Dec 29, 2022

Conversation

ncruces
Copy link
Contributor

@ncruces ncruces commented Dec 27, 2022

Fixes #958.

This is breaking a zig test, I still haven't been able to figure out why.

@ncruces
Copy link
Contributor Author

ncruces commented Dec 27, 2022

It seems, at the root of it, we have that embed.openfile does not implement io.ReaderAt. 😔

@ncruces
Copy link
Contributor Author

ncruces commented Dec 27, 2022

I could Seek, Read and then Seek back. But if any operation fails, I'm breaking the invariant of pread: “The file offset is not changed.”

@codefromthecrypt
Copy link
Contributor

@ncruces is it required for embed to implement this, or is it just due to our tests using embed?

@ncruces
Copy link
Contributor Author

ncruces commented Dec 28, 2022

No, not required. It's just due to tests using embed.

But it also means anyone using embed will start seeing pread failures. And it seems zig uses pread for most (all?) reads.

I came up with an alternative implementation that tries io.ReaderAt and falls back to io.Seeker doing multiple seeks. All tests pass. ncruces@e12b34e

There's some prior to doing this. For instance, see Go's implementation of pread under Windows:
https://github.com/golang/go/blob/e870de9936a7efa42ac1915ff4ffb16017dbc819/src/internal/poll/fd_windows.go#L538-L546

@codefromthecrypt
Copy link
Contributor

thanks for the notes and the research leading to them!

@codefromthecrypt
Copy link
Contributor

If you want to go with an alternative fix, works for me, @achille-roussel will be starting some more abstraction work soon, and the important part is to have tests in to make sure this doesn't re-break later.

@ncruces
Copy link
Contributor Author

ncruces commented Dec 28, 2022

I'll go with this since at least it means no regression for zig folks.

The big issue is this leaving the file offset in an inconsistent state if the initial seeks succeed but the final one fails. It's still an improvement over the current situation (final offset is always wrong).

Other considerations don't really apply (pread is supposed to be thread-safe, but there's no concurrency in WASM). I can add some comments pointing this out.

I'll work on tests now.

@codefromthecrypt
Copy link
Contributor

@ncruces sounds good. we're going to cut the release in a couple days, and this is great to go in. You can add notes in RATIONALE.md and then code comment to there on the tradeoffs. you'll notice there are others.

@codefromthecrypt
Copy link
Contributor

work in progress looks excellent! I'll leave you to continue with stdlib tests unless you've run out of steam.

@codefromthecrypt
Copy link
Contributor

I drifted you. for names, use export (capitalize first letter) and dot import like so . "github.com/tetratelabs/wazero/internal/wasi_snapshot_preview1"

Copy link
Contributor

@codefromthecrypt codefromthecrypt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great and professional work!

if you like I can fix the drift I caused.

Signed-off-by: Nuno Cruces <[email protected]>
Signed-off-by: Nuno Cruces <[email protected]>
@codefromthecrypt codefromthecrypt merged commit 3ab68ed into tetratelabs:main Dec 29, 2022
@codefromthecrypt
Copy link
Contributor

thanks for hanging in there @ncruces!

@ncruces ncruces deleted the bug-958 branch December 29, 2022 09:28
@ncruces
Copy link
Contributor Author

ncruces commented Dec 29, 2022

No problem.
Thank you for wazero!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

WASI fd_pread incorrectly moves file offset
2 participants