pchickey edited PR #2832 from pch/wiggle_sync_shimming
to main
.
pchickey edited PR #2832 from pch/wiggle_sync_shimming
to main
.
pchickey edited PR #2832 from pch/wiggle_sync_shimming
to main
.
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 liketokio
, without taking a dependency on any sort of asynchronous runtime. Existing users ofwasi-common
will not need to make any changes.It introduces the new crate
wasi-tokio
, which is a sibling crate towasi-cap-std-sync
, and also depending onwasi-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 inwasmtime-wiggle
, use the newblock_on
configuration option in place ofasync
as introduced in #2701. This mode executes the async methods in a "dummy executor" - a mockedstd::task::Waker
which will only poll a future correctly if it immediately returnsPoll::Ready
.
wasi-common
now uses anasync_trait
to make eachWasiFile
,WasiDir
, andWasiSched
trait methodasync
. 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 addasync
in front of some fns. All of these impls are synchronous - the futures returned by these will always bePoll::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 correspondingwritable
, which have semantics corresponding to a wasiSubscription::FdRead
andFdWrite
- the futures are ready when the file is ready to read or write. These methods are intended to be used by an impl ofWasiSched::poll_oneoff
.
wasi-tokio
is implemented mostly in terms ofwasi-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 thewasi-cap-std-sync
impl are wrapped intokio::task::block_in_place
, which tells the tokio runtime to execute that code on a thread that may block. This is how tokio's ownFile
type treats all of its IO operations as well. The departure ofwasi-tokio
from wasi-cap-std-sync is in the impl of theWasiFile::{readable, writable}
methods, which usetokio::io::unix::AsyncFd
when available, and theWasiSched::poll_oneoff
method is implemented using those futures andtokio::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 withkqueue(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 differentWasi
s - one undersync::
when thesync
feature is enabled (which it is by default), and one undertokio::
when thetokio
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 usingwasmtime_wasi::tokio::Wasi
undertokio
. It incorporates a lot of comments @alexcrichton wrote previously.<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
pchickey requested alexcrichton for a review on PR #2832.
pchickey updated PR #2832 from pch/wiggle_sync_shimming
to main
.
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 liketokio
, without taking a dependency on any sort of asynchronous runtime. Existing users ofwasi-common
will not need to make any changes.It introduces the new crate
wasi-tokio
, which is a sibling crate towasi-cap-std-sync
, and also depending onwasi-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 inwasmtime-wiggle
, use the newblock_on
configuration option in place ofasync
as introduced in #2701. This mode executes the async methods in a "dummy executor" - a mockedstd::task::Waker
which will only poll a future correctly if it immediately returnsPoll::Ready
.
wasi-common
now uses anasync_trait
to make eachWasiFile
,WasiDir
, andWasiSched
trait methodasync
. 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 addasync
in front of some fns. All of these impls are synchronous - the futures returned by these will always bePoll::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 correspondingwritable
, which have semantics corresponding to a wasiSubscription::FdRead
andFdWrite
- the futures are ready when the file is ready to read or write. These methods are intended to be used by an impl ofWasiSched::poll_oneoff
.
wasi-tokio
is implemented mostly in terms ofwasi-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 thewasi-cap-std-sync
impl are wrapped intokio::task::block_in_place
, which tells the tokio runtime to execute that code on a thread that may block. This is how tokio's ownFile
type treats all of its IO operations as well. The departure ofwasi-tokio
from wasi-cap-std-sync is in the impl of theWasiFile::{readable, writable}
methods, which usetokio::io::unix::AsyncFd
when available, and theWasiSched::poll_oneoff
method is implemented using those futures andtokio::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 withkqueue(2)
. So, on Linux we are basically stuck making believe that regular files are immediately readable and writable, which is as much asselect
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 differentWasi
s - one undersync::
when thesync
feature is enabled (which it is by default), and one undertokio::
when thetokio
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 usingwasmtime_wasi::tokio::Wasi
undertokio
. It incorporates a lot of comments @alexcrichton wrote previously.<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
alexcrichton submitted PR Review.
alexcrichton submitted PR Review.
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...)
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)
alexcrichton created PR Review Comment:
I'm a little confused by this, how come there's an
asyncify
of anasync
function?Although reading the above
asyncify
again I kind of understand, but not really.
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
alexcrichton created PR Review Comment:
This isn't needed for tests though, right?
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 theasync_trait
dependency?
alexcrichton created PR Review Comment:
How come this continues polling all futures rather than just returning the first ready value immediately?
alexcrichton created PR Review Comment:
I think this technically isn't
unsafe
, right?
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 theasync
in cap-std-sync is a lie?I think this may want a more clear comment somewhere if possible?
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?
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)
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)
alexcrichton created PR Review Comment:
This can all be folded into
get_typed_func
I think?
pchickey submitted PR Review.
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.
pchickey submitted PR Review.
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.
pchickey submitted PR Review.
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.
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.
pchickey submitted PR Review.
pchickey submitted PR Review.
pchickey created PR Review Comment:
Agreed this needs a comment. The
async
in cap-std-sync is indeed a lie.
pchickey submitted PR Review.
pchickey created PR Review Comment:
Maybe I should just rename
asyncify
toblock_in_place
? I stole theasyncify
name from tokio::File internals, but it turned into something different.
pchickey edited PR Review Comment.
pchickey submitted PR Review.
pchickey created PR Review Comment:
block_on_dummy_executor
?
pchickey submitted PR Review.
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.
pchickey submitted PR Review.
pchickey created PR Review Comment:
this one is async and cap-std-syncs is sync, but yeah they can be shared locally
pchickey updated PR #2832 from pch/wiggle_sync_shimming
to main
.
pchickey updated PR #2832 from pch/wiggle_sync_shimming
to main
.
pchickey submitted PR Review.
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.
pchickey updated PR #2832 from pch/wiggle_sync_shimming
to main
.
pchickey updated PR #2832 from pch/wiggle_sync_shimming
to main
.
pchickey updated PR #2832 from pch/wiggle_sync_shimming
to main
.
pchickey updated PR #2832 from pch/wiggle_sync_shimming
to main
.
pchickey requested alexcrichton for a review on PR #2832.
pchickey updated PR #2832 from pch/wiggle_sync_shimming
to main
.
alexcrichton submitted PR Review.
pchickey updated PR #2832 from pch/wiggle_sync_shimming
to main
.
pchickey updated PR #2832 from pch/wiggle_sync_shimming
to main
.
pchickey updated PR #2832 from pch/wiggle_sync_shimming
to main
.
pchickey updated PR #2832 from pch/wiggle_sync_shimming
to main
.
pchickey updated PR #2832 from pch/wiggle_sync_shimming
to main
.
pchickey updated PR #2832 from pch/wiggle_sync_shimming
to main
.
pchickey updated PR #2832 from pch/wiggle_sync_shimming
to main
.
pchickey merged PR #2832.
Last updated: Dec 23 2024 at 12:05 UTC