Stream: git-wasmtime

Topic: wasmtime / PR #9129 wasi-filesystem: Implement `HostInput...


view this post on Zulip Wasmtime GitHub notifications bot (Aug 14 2024 at 16:10):

badeend opened PR #9129 from badeend:file-hostinputstream to bytecodealliance:main:

Fixes #9058


All input streams are implemented using the HostInputStream trait. Except files. They got special treatment. This can be seen in the definition of InputStream:

https://github.com/bytecodealliance/wasmtime/blob/ba864e987ef1ab87c439ca6b396264547d2425e1/crates/wasi/src/stream.rs#L165-L168

The special case was introduced to work around the fact that OS'es don't actually provide any _true_ async APIs for files. A more detailed explanation can be read in the PR that introduced this setup: https://github.com/bytecodealliance/wasmtime/pull/6556


This PR properly implements the HostInputStream & HostOutputStream traits for files. And with "properly" I mean:

Primary changes:

Slightly related changes:

view this post on Zulip Wasmtime GitHub notifications bot (Aug 14 2024 at 16:10):

badeend requested pchickey for a review on PR #9129.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 14 2024 at 16:10):

badeend requested wasmtime-core-reviewers for a review on PR #9129.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 15 2024 at 15:19):

alexcrichton submitted PR review:

Thanks for taking on this refactoring, I really like how it's turned out! I think that this will work quite well.

One thing I can note which I was originally worried about but am no longer is that it's still possible for guests to queue up a lot of tasks. For example if a guest queues a write and then cancels it that'll block the guest waiting to cancel that write. The host may then cancel the host guest which will cancel-the-cancel in a way meaning that the write's just sitting in the ether on a blocking thread waiting to be completed. That initially sounded bad to me but I think this is ok because Tokio has a fixed size of blocking calls which can be "in the ether" and otherwise cancellation of a job in the queue that hasn't actually run yet is successful. This means that it's not possible to get an unbounded number of writes pending at any point with this cancellation.

I've got some minor stylistic thoughts below but otherwise this looks good-to-go to me :+1:

view this post on Zulip Wasmtime GitHub notifications bot (Aug 15 2024 at 15:19):

alexcrichton created PR review comment:

The implementation of this seems a little subtle and while I see no issue with it I think it would be best to share as much as possible with the above code. Could something like this work?

That I think will keep the blocking_read bits here deduplicated with the rest and the only responsibility of blocking_read vs read would be to filter the states that read can see by doing blocking work up front.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 15 2024 at 15:19):

alexcrichton created PR review comment:

Should this perhaps await the result of the read here inline?

view this post on Zulip Wasmtime GitHub notifications bot (Aug 15 2024 at 15:19):

alexcrichton created PR review comment:

I'm a little confused by this, is it required? I would expect that reads are only initiated from the read method above (or blocking-read) and the ready bits here only wait for completion of an already pending read.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 15 2024 at 18:26):

badeend submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 15 2024 at 18:26):

badeend created PR review comment:

is it required?

Not sure, but the current wasmtime-wasi implementation would push me towards a hesitant "Yes?"
I kinda stole the idea from the stdin implementation that also initiates a new read if there's no data available:

https://github.com/bytecodealliance/wasmtime/blob/c7756bd2654fe1ac795f9dd9d13de41a31cafbac/crates/wasi/src/stdio/worker_thread_stdin.rs#L156-L175

This, combined with the fact that there exist multiple places in the code that blindly assume that calling .ready().await will yield readable data:

https://github.com/bytecodealliance/wasmtime/blob/c7756bd2654fe1ac795f9dd9d13de41a31cafbac/crates/wasi/src/host/io.rs#L178-L182
https://github.com/bytecodealliance/wasmtime/blob/c7756bd2654fe1ac795f9dd9d13de41a31cafbac/crates/wasi/src/host/io.rs#L211-L214
https://github.com/bytecodealliance/wasmtime/blob/c7756bd2654fe1ac795f9dd9d13de41a31cafbac/crates/wasi/src/host/io.rs#L231-L234
https://github.com/bytecodealliance/wasmtime/blob/c7756bd2654fe1ac795f9dd9d13de41a31cafbac/crates/wasi/src/stream.rs#L153-L154

view this post on Zulip Wasmtime GitHub notifications bot (Aug 15 2024 at 18:28):

badeend created PR review comment:

Hmm, this should've actually been unreachable!() as this state is awaited higher up in the function.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 15 2024 at 18:28):

badeend submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 15 2024 at 19:07):

badeend updated PR #9129.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 15 2024 at 19:09):

badeend submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 15 2024 at 19:09):

badeend created PR review comment:

Something like this?

view this post on Zulip Wasmtime GitHub notifications bot (Aug 15 2024 at 21:04):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 15 2024 at 21:04):

alexcrichton created PR review comment:

Perfect!

view this post on Zulip Wasmtime GitHub notifications bot (Aug 15 2024 at 21:10):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 15 2024 at 21:10):

alexcrichton created PR review comment:

Hm ok, I'm not sure if this will survive the test of time but following suit with stdin doesn't seem unreasonable. This shouldn't get hit most of the time for the time being as WASIp1 shouldn't use this. Let's go ahead and land it :+1:

view this post on Zulip Wasmtime GitHub notifications bot (Aug 15 2024 at 21:25):

alexcrichton merged PR #9129.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 15 2024 at 22:25):

pchickey commented on PR #9129:

@badeend sorry I didn't get time to review this before it landed, but thanks very much for working on it!


Last updated: Dec 23 2024 at 12:05 UTC