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

WASI Preview 2: rewrite streams and pollable implementation #6556

Merged
merged 122 commits into from
Jul 24, 2023

Conversation

pchickey
Copy link
Contributor

@pchickey pchickey commented Jun 9, 2023

When we landed the WASI Preview 2 support in wasmtime-wasi we knew the streams/poll/scheduler implementation was going to get torn out and rewritten: see task 4 in #6370. This is that rewrite. Most of this was pair-programmed with @elliottt.

Features

wasmtime-wasi Preview 2 support is now built on top of tokio, and the host implementations for streams and pollable always use async Rust. It now passes all of the poll_oneoff tests, including on windows.

The new HostInputStream, HostOutputStream, and HostPollable traits are used to implement the wasi:io/streams.{input-stream,output-stream} and wasi:poll/poll.pollable resources.

Host{Input, Output}Stream design

input-stream and output-stream resources have a significant number of methods. For the host traits, we managed to boil this down to two required methods, as well as some optional methods to cover vectored IO, splice, and write-zeroes. The default implementations provide an unoptimized but correct implementation - the user may override them if an optimization is desired.

HostInputStream has just two methods: fn read(&mut self, ...) and async fn ready(&mut self). The most important invariant is that read must be non-blocking, and ready must block until the stream is ready for reading.

Nonblocking IO is actually impossible on regular files, but we hide that detail from WASI programs

There's a huge lie in the description above: internally, wasmtime_wasi uses InternalHost{Input,Output}Stream, which are enums that contain either a Host{Input,Output}Stream, or a private File{Input,Output}Stream. These file streams always perform a blocking read/write, whether or not the WASI program has called the nonblocking read or the blocking blocking_read method. And, any pollable associated with file read or write readiness always returns ready immediately.

This is required because there is no way to implement streams on files with epoll(7) (or on macos or windows using mio, afaik) based readiness - poll(2) will lie and always tell you files are ready for reading of writing, but the actual syscall may end up blocking the thread due to disk IO. And, rather than stick the work on a background task, we must wait until the OS read(2) or write(2) for a file has completed before returning from wasi stream (nonblocking) read or write, because the userlands in WASI expect (reasonably!) that any errors in regular file IO are reported synchronously.

So, we put an escape hatch into wasmtime-wasi which is just for regular file IO. It awaits for a tokio spawn_blocking to complete in both the blocking and non-blocking implementations of the stream reads and writes. Like all the other syscalls in the filesystem implementation, we need the async rust to put any blocking syscalls in a spawn_blocking, in order to not block the executor.

We spent a the better part of a week trying to square this circle and this is the best we could come up to make files work like we expect, but also expose the right interfaces from wasmtime-wasi so that all other streams have the proper non-blocking semantics.

HostPollable design

The trick to HostPollable is to create a Rust Future indicating whether a stream (or some other resource, e.g. http bodies) is ready without requiring both the stream and pollable hold some Arc<Mutex<...>> to the resource itself. @alexcrichton pointed out the right design here: HostPollable is an enum with two variants:

  • TableEntry { index: u32, make_future: for<'a> fn(&'a mut dyn Any) -> Pin<Box<dyn Future = Result<()>> + Send + 'a>} means, get a mutable reference to some (still dynamically-typed) other entry in the table, and apply this fn to create a Future from that entry, which has the same lifetime as the mutable reference. For the streams, we use this variant, and make_future is just a call to the Host{Input,Output}Stream::ready method.
  • Closure( Box<Fn() -> Pin<Box<dyn Future = Result<()>> + Send + 'static>> + Send + Sync + 'static> ) means, use this owned closure to create a Future with a static lifetime. This is used for creating timers from the monotonic and wall clocks, which are ambient and therefore do not have table entries.

Note that we discovered a major design problem with the TableEntry variant that exposes a crash vector if the parent resource of a pollable is destroyed before the pollable itself. This is a big enough problem that we are setting it aside to be solved in a follow-up PR.

Implementations

wasmtime-wasi provides a struct AsyncReadStream which wraps a tokio::io::AsyncRead to provide an impl of HostInputStream, AsyncWriteStream to wraps AsyncWrite to provide HostOutputStream.

AsyncReadStream and AsyncWriteStream will manage their own buffer in order for the HostInputStream::read and HostOutputStream::write methods to be non-blocking. Each spawns a helper tokio::task to perform async operations in the background. This requires some buffering.

Note, we discussed ensuring these background tasks are reaped properly - right now we believe that the implementations are correct and they will exit on their own when the foreground side gets dropped. However, in the future to implement e.g. splice and forward correctly, we may need to allow these tasks to (optionally!) live longer than the WASI program's execution.

Additionally, we have provided an implementation that correctly handles waiting on stdin readiness on both windows and unix. In Unix, this is built on top of tokio::os::unix::AsyncFd, allows tokio to register stdin with epoll(7). Because stdin is a process-wide singleton, and also because epoll will return an error if the same fd is registered multiple times, the stdin resource is a global singleton, created lazily and guarded by a mutex. wasmtime_wasi::preview2::stdio::stdin() returns a struct Stdin which takes a lock on that singleton in the HostInputStream::{read, ready} methods. This means that, if for some reason you have granted the process stdin to multiple wasi streams (in the same, or different, stores) you can get all sorts of strange behavior because the same global resource backs them all behind the scenes - but this is always the case, the implementation in wasmtime_wasi just allows you to do so without hanging or panicking.

On Windows, the implementation of stdin is broken, and inheriting stdin will panic. We have a path to fixing it, but we are leaving that out of this PR, because it has been open for far too long, and far too much work is predicated on landing it.

Sync and Async uses

Despite using async for its implementation, wasmtime-wasi will still work for synchronous Wasmtime embeddings. A new test-programs suite wasi-preview2-components-sync is identical to wasi-preview2-components in all regards except that it uses wasmtime's synchronous APIs, and the appropriate wasmtime_wasi::command::sync::add_to_linker, rather than wasmtime_wasi::command::add_to_linker which requires an async Config. The synchronous implementation creates its own Tokio runtime (if one does not already exist) and then invokes the async implementation inside of a block_on.

The wasi-common Preview 1 poll_oneoff implementation allowed for a synchronous implementation to work without any sort of dependency on tokio, by abstracting it behind a pluggable WasiSched. This PR eliminates that option for our Preview 2 users. If there is a compelling need, and labor available to implement and maintain it, we could collaborate on re-introducing a fully synchronous tokio-free option in the future. For this implementation, I decided that we just need a tokio backend in order to build a path towards supporting both wasi-sockets and wasi-http with the same modular stream and pollable host traits.

Bindings traits

After living with the bindings generated underneath crate::preview2::wasi and crate::preview2::wasi::command for a while, I decided that the interface traits belong under crate::preview2::bindings and command belongs under crate::preview2::command.

Beyond that code motion, we now generate async traits for the wasi:io/streams, wasi:poll/poll, and wasi:filesystem/filesystem interfaces, and sync traits for everything else. If you want to use the bindings from an async embedding (i.e. with an async Config), the crate::preview2::bindings::{interface}::add_to_linker will do it, and if you want to use them from a synchronous embedding, use crate::preview2::bindings::sync::{interface}::add_to_linker.

@pchickey pchickey force-pushed the pch/preview2_async_streams branch from 2ac7d27 to 8b9b326 Compare June 9, 2023 21:02
@github-actions github-actions bot added the wasi Issues pertaining to WASI label Jun 9, 2023
@elliottt elliottt force-pushed the pch/preview2_async_streams branch 3 times, most recently from abf384f to c39782c Compare June 16, 2023 20:55
@elliottt elliottt force-pushed the pch/preview2_async_streams branch from c39782c to ff4e314 Compare June 20, 2023 23:46
Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

Nice 👍

For the WIT changes, those are from the upstream proposals? Or local changes made temporarily for this PR?

crates/wasi/src/preview2/pipe.rs Outdated Show resolved Hide resolved
crates/wasi/src/preview2/ctx.rs Outdated Show resolved Hide resolved
crates/wasi/src/preview2/pipe.rs Outdated Show resolved Hide resolved
crates/wasi/src/preview2/pipe.rs Outdated Show resolved Hide resolved
}
/// Provides a [`HostOutputStream`] impl from a [`tokio::io::AsyncWrite`] impl
pub struct AsyncWriteStream {
state: Option<WriteState>,
Copy link
Member

Choose a reason for hiding this comment

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

Instead of an Option here perhaps add Closed to WriteState?

Copy link
Contributor Author

@pchickey pchickey Jul 21, 2023

Choose a reason for hiding this comment

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

we had it factored that way, then we were taking advantage of Option::take() so changing the repr made sense, but now I think I can switch it back... Edit nope, we still use the take() to perform the move of the error case out of the state. it still could be changed with appropriate helper funcs but i dont think its worth it

Comment on lines +602 to +606
// The Drop will close the file/dir, but if the close syscall
// blocks the thread, I will face god and walk backwards into hell.
// tokio::fs::File just uses std::fs::File's Drop impl to close, so
// it doesn't appear anyone else has found this to be a problem.
// (Not that they could solve it without async drop...)
Copy link
Member

Choose a reason for hiding this comment

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

Had a good chuckle reading this :)

Regardless I agree we can't make this truly async and sync close here is the right thing to do IMO

@pchickey
Copy link
Contributor Author

The wit has not been submitted upstream yet. I was going to work on that once this landed, as well as the rest of the upstream/wasmtime wit reconciliation that is pending

@pchickey
Copy link
Contributor Author

I just made a big update to the PR description with the design changes since the last round of review, and the known problems in this PR. I am now going to make the windows inherit stdin panic, mark the tests that affects as cfg_attr(windows, should_panic), and land this PR. The follow up work includes, in rough priority order:

  • HostPollable::TableEntry issues around pollables outliving their parent resource, which are a crash vector right now
  • get windows stdin working, and better tests of unix asyncfd based stdin
  • implementing stream splice and forward, and dealing with the lifetimes of background tasks accordingly

@pchickey pchickey enabled auto-merge July 21, 2023 22:16
Pat Hickey added 2 commits July 21, 2023 15:41
@pchickey pchickey added this pull request to the merge queue Jul 24, 2023
Merged via the queue into main with commit 0f9ac11 Jul 24, 2023
@pchickey pchickey deleted the pch/preview2_async_streams branch July 24, 2023 04:26
geekbeast pushed a commit to geekbeast/wasmtime that referenced this pull request Aug 1, 2023
* main: (47 commits)
  Add core dump support to the runtime (bytecodealliance#6513)
  Resource table tracks child relationships (bytecodealliance#6779)
  Wasmtime: Move `OnDemandInstanceAllocator` to its own module (bytecodealliance#6790)
  wasi: Test the stdio streams implementation (bytecodealliance#6764)
  Don't generate same-named imports in fact modules (bytecodealliance#6783)
  Wasmtime: Add support for Wasm tail calls (bytecodealliance#6774)
  Cranelift: Fix `ABIMachineSpec::gen_add_imm` for riscv64 (bytecodealliance#6780)
  Update the wasm-tools family of crates, disallow empty component types (bytecodealliance#6777)
  Fix broken link to WASI API documentation (bytecodealliance#6775)
  A bunch of cleanups for cranelift-codegen-meta (bytecodealliance#6772)
  Implement component-to-component calls with resources (bytecodealliance#6769)
  Ignore async_stack_size if async_support is disabled (bytecodealliance#6771)
  A bunch of minor cleanups (bytecodealliance#6767)
  Fix flaky tests in preview2 streams (bytecodealliance#6763)
  Refactor and simplify component trampolines (bytecodealliance#6751)
  Cranelift: Implement tail calls on riscv64 (bytecodealliance#6749)
  WASI Preview 2: rewrite streams and pollable implementation (bytecodealliance#6556)
  cranelift-wasm: Add support for translating Wasm tail calls to CLIF (bytecodealliance#6760)
  Cranelift: Get tail calls working on aarch64 (bytecodealliance#6723)
  Implement component model resources in Wasmtime (bytecodealliance#6691)
  ...
rvolosatovs pushed a commit to rvolosatovs/wasi-io that referenced this pull request Aug 24, 2023
…alliance/wasmtime#6556)

* preview2: make everything but streams/io and poll/poll synchronous

* streams: get rid of as_any method, which is no longer used

* delete legacy sched and pollable concepts

* more code motion and renaming

* make tokio a workspace dep, because we need it directly in wasmtime-wasi

* HostPollable exists

* more fixes

* pollable can trap, and implement clock properly

* HostPollable is now a generator of futures

because we need to be able to poll a pollable many times

* explain various todo!s

* Synchronous version of the wasi-preview2-components tests

* Change with_tokio to accept the future as an argument

* Store futures in the PollOneoff struct instead, to avoid dropping them

* Remove TODO for HostOutputStream impl for WritePipe

* Implement pollable for ReadPipe

* Use a Notify when ReadPipe is ready

* wip

* wip

* Read/write pipe ends with tokio channels

* Empty reader/writer wrappers

* EmptyStream, and warning cleanup

* Wrapped reader/writer structs

* Rework stdio in terms of wrapped read/write

* Add MemoryOutputPipe and update tests

* Remove todo

* rewrite nearly everything

* implement the pipe stuff

* wibble

* fix MemoryOutputPipe just enough to make the tests compile

* Move the table iteration into a helper function

* AsyncFd stream implementation to fix stdin on unix

* Rename Wrapped{Read,Write} streams to Async{Read,Write}Stream

* Move async io wrappers into stream.rs

* Fix the sync tests

* fix test uses of pipes, juggle tokio context for stdin construction

* add some fixmes

* the future i named Never is defined in futures-util as pending

which is a better name

* i believe this is a correct implementation of one global stdin resource

* move unix stdin to its own file

* make most of the mods private

* fix build - we are skipping rust 1.70

due to llvm regressions in s390x and riscv64 which are fixed in 1.71 and
will not be backported

* preview1-in-preview2: use async funcs for io, and the async io interface

prtest:full

* windows stdin support

* done!

* table ext functions: fix tests

* tests: expect poll_oneoff_{files,stdio} to pass on all platforms

* export the bindings under wasmtime_wasi::preview2::bindings

rather than preview2::wasi.

and command moves to wasmtime_wasi::preview2::command as well.

* fix renaming of wasi to bindings in tests

* use block_in_place throughout filesystem

and move block_on and block_in_place to be pub crate at the root

* AsyncFdStream: ensure file is nonblocking

* tests: block_in_place requires multi-threaded runtime

* actually, use fcntl_setfl to make the asyncfd file nonblocking

* fix windows block_on

* docs, remove unnecessary methods

* more docs

* Add a workspace dependency on bytes-1.4

* Remove vectored stream operations

* Rework the read/write stream traits

* Add a size parameter to `read`, and switch to usize for traits

* Pipe through the bool -> stream-status change in wit

* Plumb stream-status through write operations in wit

* write host trait also gives streamstate

* hook new stream host read/write back up to the wit bindgen

* sketchy AsyncReadStream impl

* Fill out implementations for AsyncReadStream and AsyncWriteStream

* some reasonable read tests

* more

* first smoke test for AsyncWriteStream

* bunch of AsyncWriteStream tests

* half-baked idea that the output-stream interface will need a flush mechanism

* adapter: fixes for changes to stream wit

* fix new rust 1.71 warnings

* make stdin work on unix without using AsyncFdStream

inline the tokio docs example of how to impl AsyncRead for an AsyncFd,
except theres some "minor" changes because stdin doesnt impl Read on
&Stdin whereas tcpstream from the example does

* delete AsyncFdStream for now

it turns out to be kinda hard and we can always work on adding it back
in later.

* Implement some memory pipe operations, and move async wrappers to the pipe mod

* Make blocking_write actually block until everything is written

* Remove debug print

* Adapter stdio should use blocking write

Rust guests will panic if the write returns less than the number of
bytes sent with stdio.

* Clean up implementations of {blocking_}write_zeros and skip

* Remove debug macro usage

* Move EmptyStream to pipe, and split it into four variants

Use EmptyInputStream and SinkOutputStream as the defaults for stdin and
stdout/stderr respectively.

* Add a big warning about resource lifetime tracking in pollables

* Start working through changes to the filesystem implementation

* Remove todos in the filesystem implementation

* Avoid lifetime errors by moving blocking operations to File and Dir

* Fix more lifetime issues with `block`

* Finish filling out translation impl

* fix warnings

* we can likely eliminate block_in_place in the stdin implementations

* sync command uses sync filesystem, start of translation layer

* symc filesystem: all the trait boilerplate is in place

just need to finish the from impl boilerplate

* finish type conversion boilerplate

* Revert "half-baked idea that the output-stream interface will need a flush mechanism"

This reverts commit 3eb762e3330a7228318bfe01296483b52d0fdc16.

* cargo fmt

* test type fixes

* renames and comments

* refactor stream table internals so we can have a blocking variant...

* preview1 host adapter: stdout/stderr use blocking_write here too

* filesystem streams are blocking now

* fixes

* satisfy cargo doc

* cargo vet: dep upgrades taken care of by imports from mozilla

* unix stdio: eliminate block_in_place

* replace private in_tokio with spawn, since its only used for spawning

* comments

* worker thread stdin implementation can be tested on linux, i guess

and start outlining a test plan

* eliminate tokio boilerplate - no longer using tokios lock

* rename our private block_on to in_tokio

* fill in missing file input skip

* code review: fix MemoryInputPipe. Closed status is always available immediately.

* code review: empty input stream is not essential, closed input stream is a better fi for stdin

* code review: unreachable

* turn worker thread (windows) stdin off

* expect preview2-based poll_oneoff_stdio to fail on windows

* command directory_list test: no need to inherit stdin

* preview1 in preview2: turn off inherit_stdio except for poll_oneoff_stdio

* wasi-preview2-components: apparently inherit_stdio was on everywhere here as well. turn it off

except for poll_oneoff_stdio

* extend timeout for riscv64 i suppose

---------

Co-authored-by: Trevor Elliott <telliott@fastly.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wasi Issues pertaining to WASI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants