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

Write all iovs in fd_write of wasi-preview1-component-adapter #8072

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ospencer
Copy link
Member

@ospencer ospencer commented Mar 9, 2024

I noticed that the implementation of fd_write in the preview1 adapter only writes the first iov in the list passed. This change writes all of them. I'm happy to add a test for this, though I couldn't find any tests for the adapter. I tested this locally via wasm-tools.

@ospencer ospencer requested a review from a team as a code owner March 9, 2024 00:30
@ospencer ospencer requested review from alexcrichton and removed request for a team March 9, 2024 00:30
@bjorn3
Copy link
Contributor

bjorn3 commented Mar 9, 2024

Writing in a loop is incompatible with POSIX. POSIX mandates that each writev is an atomic operation. While it may only write the first n bytes, it has to write all n bytes at once without the possibility of any other write getting interleaved with it. If you want to write everything, you did have to copy all buffers into one large buffer and then write this large buffer, which may actually be slower than both the status quo and your PR. See also #8037 (comment)

@alexcrichton
Copy link
Member

To add another case I would be worried about in addition to what @bjorn3 already mentioned: in the PR as-is if you have 4 buffers and successfully write the first two but then fail writing the third then the fact that the first two were successfully written is lost. At that point you've got a number of successfully written bytes plus an error and it's not clear which should be returned since only one can be returned.

@ospencer
Copy link
Member Author

That makes sense. It looks like some implementations of readv/writev do allocate a temporary large buffer in the case that the number of provided iovs exceeds the limit of the system, so that probably makes sense here too.

I agree that the current implementation we have of fd_write is technically correct, but I think the (totally allowable) behavior change from older preview1 versions of wasmtime is a little surprising to less resilient code, shown by the couple of issues/PRs opened about this.

I attempted to implement this so we could at least test it out/consider this approach, but I forgot that the adapter can't really use Vecs because of the panics. I'm not quite a seasoned-enough Rustacean to know how to do this without vecs in safe Rust, but if someone could give me a pointer that'd be much appreciated.

@alexcrichton
Copy link
Member

I agree it's unfortunate that this is causing issues, but IMO our hands are unfortunately tied here to the point that there's not much we can do. I think there's one thing we can do, which is to have a statically allocated or somewhere slice of bytes which the adapter copies into and then calls out to perform a single write. That would respect the semantics desired for languages and such here, at the cost of copying data. It's not clear to me when the copy is worth it vs when not, so I'm not sure how to best make such a decision.

The adapter has various bits and pieces of scratch space in its State and we can put a fixed-size buffer in there for copying if necessary.

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.

None yet

3 participants