wasmtime / PR #2832 wasi-common support for tokio, & wigg... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #2832 wasi-common support for tokio, & wigg...

Wasmtime GitHub notifications bot (May 05 2021 at 23:19):

pchickey edited PR #2832 from pch/wiggle_sync_shimming to main.

Wasmtime GitHub notifications bot (May 05 2021 at 23:23):

pchickey edited PR #2832 from pch/wiggle_sync_shimming to main.

Wasmtime GitHub notifications bot (May 05 2021 at 23:23):

pchickey edited PR #2832 from pch/wiggle_sync_shimming to main.

Wasmtime GitHub notifications bot (May 06 2021 at 00:01):

pchickey edited PR #2832 from pch/wiggle_sync_shimming to main:

This PR makes it possible for wasi-common to work on with async executor like tokio, without taking a dependency on any sort of asynchronous runtime. Existing users of wasi-common will not need to make any changes.

It introduces the new crate wasi-tokio, which is a sibling crate to wasi-cap-std-sync, and also depending on wasi-cap-std-sync, since much of the implementation has been reused.

wasmtime-wiggle now supports running async methods via a dummy executor, in order to support running synchronous wasi-common implementations. To configure this mode in wasmtime-wiggle, use the new block_on configuration option in place of async as introduced in #2701. This mode executes the async methods in a "dummy executor" - a mocked std::task::Waker which will only poll a future correctly if it immediately returns Poll::Ready.

wasi-common now uses an async_trait to make each WasiFile, WasiDir, and WasiSched trait method async. This means that any operation on files, directories, or the scheduler may return a pending future depending on the impl of those traits.

wasi-cap-std-sync stays the same, modulo the code golf required to add async in front of some fns. All of these impls are synchronous - the futures returned by these will always be Poll::Ready. Therefore, it is safe to use with the dummy executor.

The WasiFile trait has two additional methods, async fn readable(&mut self) -> Result<(), Error> and a corresponding writable, which have semantics corresponding to a wasi Subscription::FdRead and FdWrite - the futures are ready when the file is ready to read or write. These methods are intended to be used by an impl of WasiSched::poll_oneoff.

wasi-tokio is implemented mostly in terms of wasi-cap-std-sync. cap-std provides a robust implementation of filesystem sandboxing, which we still need to rely on. All file and directory operations delegated to the wasi-cap-std-sync impl are wrapped in tokio::task::block_in_place, which tells the tokio runtime to execute that code on a thread that may block. This is how tokio's own File type treats all of its IO operations as well. The departure of wasi-tokio from wasi-cap-std-sync is in the impl of the WasiFile::{readable, writable} methods, which use tokio::io::unix::AsyncFd when available, and the WasiSched::poll_oneoff method is implemented using those futures and tokio::time::timeout.

Unfortunately, tokio's AsyncFd doesn't do very much in this context on Linux (epoll(2) doesn't operate on regular files), and isn't available at all on Windows, but at least on Mac OS it does work well with kqueue(2). So, on Linux we are basically stuck making believe that regular files are immediately readable and writable, and on Windows we fall back on using the cap-std-sync windows poll_oneoff inside a block_in_place.

wasmtime-wasi now exports two different Wasis - one under sync:: when the sync feature is enabled (which it is by default), and one under tokio:: when the tokio feature is enabled (not a default). The sync implementation is re-exported at the root of the crate, so that existing users do not have to change their code.

A new top-level examples/tokio shows using wasmtime_wasi::tokio::Wasi under tokio. It incorporates a lot of comments @alexcrichton wrote previously.

Wasmtime GitHub notifications bot (May 06 2021 at 00:02):

pchickey requested alexcrichton for a review on PR #2832.

Wasmtime GitHub notifications bot (May 06 2021 at 00:03):

pchickey updated PR #2832 from pch/wiggle_sync_shimming to main.

Wasmtime GitHub notifications bot (May 06 2021 at 00:04):

pchickey edited PR #2832 from pch/wiggle_sync_shimming to main:

This PR makes it possible for wasi-common to work on with async executor like tokio, without taking a dependency on any sort of asynchronous runtime. Existing users of wasi-common will not need to make any changes.

It introduces the new crate wasi-tokio, which is a sibling crate to wasi-cap-std-sync, and also depending on wasi-cap-std-sync, since much of the implementation has been reused.

wasmtime-wiggle now supports running async methods via a dummy executor, in order to support running synchronous wasi-common implementations. To configure this mode in wasmtime-wiggle, use the new block_on configuration option in place of async as introduced in #2701. This mode executes the async methods in a "dummy executor" - a mocked std::task::Waker which will only poll a future correctly if it immediately returns Poll::Ready.

wasi-common now uses an async_trait to make each WasiFile, WasiDir, and WasiSched trait method async. This means that any operation on files, directories, or the scheduler may return a pending future depending on the impl of those traits.

wasi-cap-std-sync stays the same, modulo the code golf required to add async in front of some fns. All of these impls are synchronous - the futures returned by these will always be Poll::Ready. Therefore, it is safe to use with the dummy executor.

The WasiFile trait has two additional methods, async fn readable(&mut self) -> Result<(), Error> and a corresponding writable, which have semantics corresponding to a wasi Subscription::FdRead and FdWrite - the futures are ready when the file is ready to read or write. These methods are intended to be used by an impl of WasiSched::poll_oneoff.

wasi-tokio is implemented mostly in terms of wasi-cap-std-sync. cap-std provides a robust implementation of filesystem sandboxing, which we still need to rely on. All file and directory operations delegated to the wasi-cap-std-sync impl are wrapped in tokio::task::block_in_place, which tells the tokio runtime to execute that code on a thread that may block. This is how tokio's own File type treats all of its IO operations as well. The departure of wasi-tokio from wasi-cap-std-sync is in the impl of the WasiFile::{readable, writable} methods, which use tokio::io::unix::AsyncFd when available, and the WasiSched::poll_oneoff method is implemented using those futures and tokio::time::timeout.

Unfortunately, tokio's AsyncFd doesn't do very much in this context on Linux (epoll(2) doesn't operate on regular files), and isn't available at all on Windows, but at least on Mac OS it does work well with kqueue(2). So, on Linux we are basically stuck making believe that regular files are immediately readable and writable, which is as much as select does for us anyway. The futures do work properly on pipes like stdin. On Windows, tokio doesn't give us anything like AsyncFd, so we fall back on using the cap-std-sync windows poll_oneoff inside a block_in_place.

wasmtime-wasi now exports two different Wasis - one under sync:: when the sync feature is enabled (which it is by default), and one under tokio:: when the tokio feature is enabled (not a default). The sync implementation is re-exported at the root of the crate, so that existing users do not have to change their code.

A new top-level examples/tokio shows using wasmtime_wasi::tokio::Wasi under tokio. It incorporates a lot of comments @alexcrichton wrote previously.

Wasmtime GitHub notifications bot (May 06 2021 at 14:46):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (May 06 2021 at 14:46):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (May 06 2021 at 14:46):

alexcrichton created PR Review Comment:

This is my personal first time seeing this, although I see that this is simply copied over from the cap-std-sync version. Could this be deduplicated between those two versions perhaps? (also if you know what this does can you add a comment here about that? I have no idea what this is configuring...)

Wasmtime GitHub notifications bot (May 06 2021 at 14:46):

alexcrichton created PR Review Comment:

We probably don't need to be so general here, could this detect maybe "async" in the name and pass a hardcoded feature?

(mostly trying to keep the examples dir relatively clean)

Wasmtime GitHub notifications bot (May 06 2021 at 14:46):

alexcrichton created PR Review Comment:

I'm a little confused by this, how come there's an asyncify of an async function?

Although reading the above asyncify again I kind of understand, but not really.

Wasmtime GitHub notifications bot (May 06 2021 at 14:46):

alexcrichton created PR Review Comment:

Similar to above it seems like much of this could probably be shared between at least the above function, if not with the cap-std-sync verison too ideally

Wasmtime GitHub notifications bot (May 06 2021 at 14:46):

alexcrichton created PR Review Comment:

This isn't needed for tests though, right?

Wasmtime GitHub notifications bot (May 06 2021 at 14:46):

alexcrichton created PR Review Comment:

I think this is only used for the async_trait macro, right? If that's the case could this be just the async_trait dependency?

Wasmtime GitHub notifications bot (May 06 2021 at 14:46):

alexcrichton created PR Review Comment:

How come this continues polling all futures rather than just returning the first ready value immediately?

Wasmtime GitHub notifications bot (May 06 2021 at 14:46):

alexcrichton created PR Review Comment:

I think this technically isn't unsafe, right?

Wasmtime GitHub notifications bot (May 06 2021 at 14:46):

alexcrichton created PR Review Comment:

I think I'm getting a bit lost in my head thinking about this. This feels pretty backwards where we're "asyncifying" already async code, and I'm not sure that I have enough context for untangling why the backwards feeling is actually ok here.

The idea here is that wasi-common requires everything to be async, but then the "sync" versions just do all the blocking work in their async impls. Then when tokio builds on top of the sync versions again it just rewraps everything in block_in_place? Basically the async in cap-std-sync is a lie?

I think this may want a more clear comment somewhere if possible?

Wasmtime GitHub notifications bot (May 06 2021 at 14:46):

alexcrichton created PR Review Comment:

I'm sort of confused how this works where the unix version does a bunch of fancy "first ready" stuff but on Windows we skip all that?

Wasmtime GitHub notifications bot (May 06 2021 at 14:46):

alexcrichton created PR Review Comment:

This seems like a pretty intense file that has a helper thread to work with stdio on Windows, could this get commented with a high level overview of how all this works?

(I'm not actually sure off the top of my head why this is necessary)

Wasmtime GitHub notifications bot (May 06 2021 at 14:46):

alexcrichton created PR Review Comment:

Could this function be shortened to basically "FIXME needs https://github.com/alexcrichton/rfcs-2/blob/new-api/accepted/new-api.md to be safe" or something along those lines?

(probably not too worth maintaining docs here about the unsafety)

Wasmtime GitHub notifications bot (May 06 2021 at 14:46):

alexcrichton created PR Review Comment:

This can all be folded into get_typed_func I think?

Wasmtime GitHub notifications bot (May 06 2021 at 15:59):

pchickey submitted PR Review.

Wasmtime GitHub notifications bot (May 06 2021 at 15:59):

pchickey created PR Review Comment:

Sure, I could do that. I figured that putting the features in a file would be more discoverable for someone trying to run it standalone than detecting a part of the name.

Wasmtime GitHub notifications bot (May 06 2021 at 16:01):

pchickey submitted PR Review.

Wasmtime GitHub notifications bot (May 06 2021 at 16:01):

pchickey created PR Review Comment:

Yep thats the only use. I arbitrarily decided to import it via wiggle everywhere so we wouldn't have to keep versions synced up, but realistically I don't expect async_trait to change meaningfully at this point so I could change to importing it directly.

Wasmtime GitHub notifications bot (May 06 2021 at 16:02):

pchickey submitted PR Review.

Wasmtime GitHub notifications bot (May 06 2021 at 16:02):

pchickey created PR Review Comment:

In the corner case where several of your subscriptions are ready immediately, the tests expect all of them to be reported rather than just the first. Reporting is part of the future being polled on here, so I want all of them to get a chance to run before returning. I will improve the comment.

Wasmtime GitHub notifications bot (May 06 2021 at 16:03):

pchickey created PR Review Comment:

This code is borrowed from cap-std-sync which is in turn borrowed from the old version of wasi-common. I also don't quite understand it, and this code was highly resistant to some earlier attempts to improve it, so I have just been dragging it along. I can add better comments, though.

Wasmtime GitHub notifications bot (May 06 2021 at 16:03):

pchickey submitted PR Review.

Wasmtime GitHub notifications bot (May 06 2021 at 16:05):

pchickey submitted PR Review.

Wasmtime GitHub notifications bot (May 06 2021 at 16:05):

pchickey created PR Review Comment:

Agreed this needs a comment. The async in cap-std-sync is indeed a lie.

Wasmtime GitHub notifications bot (May 06 2021 at 16:09):

pchickey submitted PR Review.

Wasmtime GitHub notifications bot (May 06 2021 at 16:09):

pchickey created PR Review Comment:

Maybe I should just rename asyncify to block_in_place? I stole the asyncify name from tokio::File internals, but it turned into something different.

Wasmtime GitHub notifications bot (May 06 2021 at 16:10):

pchickey edited PR Review Comment.

Wasmtime GitHub notifications bot (May 06 2021 at 16:14):

pchickey submitted PR Review.

Wasmtime GitHub notifications bot (May 06 2021 at 16:14):

pchickey created PR Review Comment:

block_on_dummy_executor?

Wasmtime GitHub notifications bot (May 06 2021 at 16:16):

pchickey submitted PR Review.

Wasmtime GitHub notifications bot (May 06 2021 at 16:16):

pchickey created PR Review Comment:

Probably not - there is some unsafe under the hood but afaict that shouldnt make it unsafe itself. I debated about that keyword and ultimately threw it in there to flag "don't use this unless you are really sure", initially this was totally private code but I needed to reuse it in several crates.

Wasmtime GitHub notifications bot (May 06 2021 at 18:08):

pchickey submitted PR Review.

Wasmtime GitHub notifications bot (May 06 2021 at 18:08):

pchickey created PR Review Comment:

this one is async and cap-std-syncs is sync, but yeah they can be shared locally

Wasmtime GitHub notifications bot (May 06 2021 at 21:34):

pchickey updated PR #2832 from pch/wiggle_sync_shimming to main.

Wasmtime GitHub notifications bot (May 06 2021 at 22:46):

pchickey updated PR #2832 from pch/wiggle_sync_shimming to main.

Wasmtime GitHub notifications bot (May 06 2021 at 22:52):

pchickey submitted PR Review.

Wasmtime GitHub notifications bot (May 06 2021 at 22:52):

pchickey created PR Review Comment:

I ended up refactoring this to re-use the other implementation without copy-paste, filed #2880, and added some comments.

Wasmtime GitHub notifications bot (May 06 2021 at 22:55):

pchickey updated PR #2832 from pch/wiggle_sync_shimming to main.

Wasmtime GitHub notifications bot (May 06 2021 at 23:19):

pchickey updated PR #2832 from pch/wiggle_sync_shimming to main.

Wasmtime GitHub notifications bot (May 06 2021 at 23:23):

pchickey updated PR #2832 from pch/wiggle_sync_shimming to main.

Wasmtime GitHub notifications bot (May 06 2021 at 23:25):

pchickey updated PR #2832 from pch/wiggle_sync_shimming to main.

Wasmtime GitHub notifications bot (May 06 2021 at 23:25):

pchickey requested alexcrichton for a review on PR #2832.

Wasmtime GitHub notifications bot (May 07 2021 at 00:54):

pchickey updated PR #2832 from pch/wiggle_sync_shimming to main.

Wasmtime GitHub notifications bot (May 07 2021 at 14:42):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (May 07 2021 at 19:20):

pchickey updated PR #2832 from pch/wiggle_sync_shimming to main.

Wasmtime GitHub notifications bot (May 07 2021 at 21:28):

pchickey updated PR #2832 from pch/wiggle_sync_shimming to main.

Wasmtime GitHub notifications bot (May 07 2021 at 22:19):

pchickey updated PR #2832 from pch/wiggle_sync_shimming to main.

Wasmtime GitHub notifications bot (May 07 2021 at 22:42):

pchickey updated PR #2832 from pch/wiggle_sync_shimming to main.

Wasmtime GitHub notifications bot (May 07 2021 at 22:51):

pchickey updated PR #2832 from pch/wiggle_sync_shimming to main.

Wasmtime GitHub notifications bot (May 07 2021 at 23:07):

pchickey updated PR #2832 from pch/wiggle_sync_shimming to main.

Wasmtime GitHub notifications bot (May 07 2021 at 23:20):

pchickey updated PR #2832 from pch/wiggle_sync_shimming to main.

Wasmtime GitHub notifications bot (May 08 2021 at 00:43):

pchickey merged PR #2832.

Last updated: Oct 23 2024 at 20:03 UTC