`fd_fdstat_set_flags` requires mutable access to the file descriptor table #5643

abrown · 2023-01-27T01:20:45Z

In #5326, we discussed extensively how to resolve the build errors related to fd_fdstat_set_flags, a wasi-common function. When attempting to apply locking to wasi-common to enable multi-threaded access, this function was the sole remaining issue preventing the implementation of WasiSnapshotPreview1 from using &self (what we wanted to switch to) instead of the current &mut self. If fd_fdstat_set_flags could take &self instead, this would result in a variety of benefits:

there would be no need to wrap WasiCtx (and friends) in an inner structure containing an Arc; the Arc could be applied to the top-level Host structure instead
all of WasiSnapshotPreview1 could be implemented in terms of &self instead of &mut self
several supporting derive(Clone) and mutating functions could go away

At some point I had heard of discussions to modify this API; if and when that happens, the above improvements could be made.

The text was updated successfully, but these errors were encountered:

In order to make progress with wasi-threads, this change temporarily works around limitations induced by `wasi-common`'s `fd_fdstat_set_flags` to allow `&mut self` use in the implementation. Eventual resolution is tracked in bytecodealliance#5643. This change makes several related helper functions (e.g., `set_fdflags`) take `&mut self` as well. Co-authored-by: Alex Crichton <alex@alexcrichton.com>

This commit includes a set of changes that add initial support for `wasi-threads` to Wasmtime: * feat: remove mutability from the WasiCtx Table This patch adds interior mutability to the WasiCtx Table and the Table elements. Major pain points: * `File` only needs `RwLock<cap_std::fs::File>` to implement `File::set_fdflags()` on Windows, because of [1] * Because `File` needs a `RwLock` and `RwLock*Guard` cannot be hold across an `.await`, The `async` from `async fn num_ready_bytes(&self)` had to be removed * Because `File` needs a `RwLock` and `RwLock*Guard` cannot be dereferenced in `pollable`, the signature of `fn pollable(&self) -> Option<rustix::fd::BorrowedFd>` changed to `fn pollable(&self) -> Option<Arc<dyn AsFd + '_>>` [1] https://github.com/bytecodealliance/system-interface/blob/da238e324e752033f315f09c082ad9ce35d42696/src/fs/fd_flags.rs#L210-L217 * wasi-threads: add an initial implementation This change is a first step toward implementing `wasi-threads` in Wasmtime. We may find that it has some missing pieces, but the core functionality is there: when `wasi::thread_spawn` is called by a running WebAssembly module, a function named `wasi_thread_start` is found in the module's exports and called in a new instance. The shared memory of the original instance is reused in the new instance. This new WASI proposal is in its early stages and details are still being hashed out in the [spec] and [wasi-libc] repositories. Due to its experimental state, the `wasi-threads` functionality is hidden behind both a compile-time and runtime flag: one must build with `--features wasi-threads` but also run the Wasmtime CLI with `--wasm-features threads` and `--wasi-modules experimental-wasi-threads`. One can experiment with `wasi-threads` by running: ```console $ cargo run --features wasi-threads -- \ --wasm-features threads --wasi-modules experimental-wasi-threads \ <a threads-enabled module> ``` Threads-enabled Wasm modules are not yet easy to build. Hopefully this is resolved soon, but in the meantime see the use of `THREAD_MODEL=posix` in the [wasi-libc] repository for some clues on what is necessary. Wiggle complicates things by requiring the Wasm memory to be exported with a certain name and `wasi-threads` also expects that memory to be imported; this build-time obstacle can be overcome with the `--import-memory --export-memory` flags only available in the latest Clang tree. Due to all of this, the included tests are written directly in WAT--run these with: ```console $ cargo test --features wasi-threads -p wasmtime-cli -- cli_tests ``` [spec]: https://github.com/WebAssembly/wasi-threads [wasi-libc]: https://github.com/WebAssembly/wasi-libc This change does not protect the WASI implementations themselves from concurrent access. This is already complete in previous commits or left for future commits in certain cases (e.g., wasi-nn). * wasi-threads: factor out process exit logic As is being discussed [elsewhere], either calling `proc_exit` or trapping in any thread should halt execution of all threads. The Wasmtime CLI already has logic for adapting a WebAssembly error code to a code expected in each OS. This change factors out this logic to a new function, `maybe_exit_on_error`, for use within the `wasi-threads` implementation. This will work reasonably well for CLI users of Wasmtime + `wasi-threads`, but embedders will want something better in the future: when a `wasi-threads` threads fails, they may not want their application to exit. Handling this is tricky, because it will require cancelling the threads spawned by the `wasi-threads` implementation, something that is not trivial to do in Rust. With this change, we defer that work until later in order to provide a working implementation of `wasi-threads` for experimentation. [elsewhere]: WebAssembly/wasi-threads#17 * review: work around `fd_fdstat_set_flags` In order to make progress with wasi-threads, this change temporarily works around limitations induced by `wasi-common`'s `fd_fdstat_set_flags` to allow `&mut self` use in the implementation. Eventual resolution is tracked in #5643. This change makes several related helper functions (e.g., `set_fdflags`) take `&mut self` as well. * test: use `wait`/`notify` to improve `threads.wat` test Previously, the test simply executed in a loop for some hardcoded number of iterations. This changes uses `wait` and `notify` and atomic operations to keep track of when the spawned threads are done and join on the main thread appropriately. * various fixes and tweaks due to the PR review --------- Signed-off-by: Harald Hoyer <harald@profian.com> Co-authored-by: Harald Hoyer <harald@profian.com> Co-authored-by: Alex Crichton <alex@alexcrichton.com>

alexcrichton added the wasm-proposal:threads Issues related to the WebAssembly threads proposal label Jan 31, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`fd_fdstat_set_flags` requires mutable access to the file descriptor table #5643

`fd_fdstat_set_flags` requires mutable access to the file descriptor table #5643

abrown commented Jan 27, 2023

fd_fdstat_set_flags requires mutable access to the file descriptor table #5643

fd_fdstat_set_flags requires mutable access to the file descriptor table #5643

Comments

abrown commented Jan 27, 2023

`fd_fdstat_set_flags` requires mutable access to the file descriptor table #5643

`fd_fdstat_set_flags` requires mutable access to the file descriptor table #5643