From 2ba9f11b56adaef32f5bc2596cafab8328e52ae4 Mon Sep 17 00:00:00 2001 From: Andrew Brown Date: Mon, 9 Jan 2023 11:39:02 -0800 Subject: [PATCH] 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]: https://github.com/WebAssembly/wasi-threads/pull/17 --- Cargo.lock | 3 ++- Cargo.toml | 1 - crates/wasi-threads/Cargo.toml | 1 + crates/wasi-threads/src/lib.rs | 16 +++++------- crates/wasi/Cargo.toml | 1 + crates/wasi/src/lib.rs | 42 +++++++++++++++++++++++++++++++ src/commands/run.rs | 45 ++++++---------------------------- 7 files changed, 59 insertions(+), 50 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index dcff7ff84050..5ffacccd8668 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3477,7 +3477,6 @@ dependencies = [ "env_logger 0.9.0", "filecheck", "humantime 2.1.0", - "libc", "listenfd", "log", "memchr", @@ -3741,6 +3740,7 @@ name = "wasmtime-wasi" version = "6.0.0" dependencies = [ "anyhow", + "libc", "wasi-cap-std-sync", "wasi-common", "wasi-tokio", @@ -3778,6 +3778,7 @@ dependencies = [ "rand 0.8.5", "wasi-common", "wasmtime", + "wasmtime-wasi", ] [[package]] diff --git a/Cargo.toml b/Cargo.toml index 05a8cfc1d785..e80aa165c491 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -35,7 +35,6 @@ wasmtime-wasi-threads = { workspace = true, optional = true } clap = { workspace = true, features = ["color", "suggestions", "derive"] } anyhow = { workspace = true } target-lexicon = { workspace = true } -libc = "0.2.60" humantime = "2.0.0" once_cell = { workspace = true } listenfd = "1.0.0" diff --git a/crates/wasi-threads/Cargo.toml b/crates/wasi-threads/Cargo.toml index 6d212721464c..2667587b83df 100644 --- a/crates/wasi-threads/Cargo.toml +++ b/crates/wasi-threads/Cargo.toml @@ -17,6 +17,7 @@ log = { workspace = true } rand = "0.8" wasi-common = { workspace = true } wasmtime = { workspace = true } +wasmtime-wasi = { workspace = true } [badges] maintenance = { status = "experimental" } diff --git a/crates/wasi-threads/src/lib.rs b/crates/wasi-threads/src/lib.rs index af9f2e4458f1..88675c1ca209 100644 --- a/crates/wasi-threads/src/lib.rs +++ b/crates/wasi-threads/src/lib.rs @@ -5,8 +5,8 @@ use anyhow::{anyhow, Result}; use rand::Rng; use std::thread; -use wasi_common::I32Exit; use wasmtime::{Caller, Linker, Module, SharedMemory, Store}; +use wasmtime_wasi::maybe_exit_on_error; // This name is a function export designated by the wasi-threads specification: // https://github.com/WebAssembly/wasi-threads/#detailed-design-discussion @@ -63,15 +63,11 @@ impl WasiThreadsCtx { ); match thread_entry_point.call(&mut store, (wasi_thread_id, thread_start_arg)) { Ok(_) => {} - Err(trap) => { - if let Some(I32Exit(0)) = trap.downcast_ref::() { - log::trace!( - "exited thread id = {} via `wasi::proc_exit` call", - wasi_thread_id - ) - } else { - panic!("{}", fail(trap.to_string())) - } + Err(e) => { + log::trace!("exiting thread id = {} due to error", wasi_thread_id); + let e = maybe_exit_on_error(e); + eprintln!("Error: {:?}", e); + std::process::exit(1); } } })?; diff --git a/crates/wasi/Cargo.toml b/crates/wasi/Cargo.toml index cca14f9597ce..73649f4afabd 100644 --- a/crates/wasi/Cargo.toml +++ b/crates/wasi/Cargo.toml @@ -13,6 +13,7 @@ include = ["src/**/*", "README.md", "LICENSE", "build.rs"] build = "build.rs" [dependencies] +libc = "0.2.60" wasi-common = { workspace = true } wasi-cap-std-sync = { workspace = true, optional = true } wasi-tokio = { workspace = true, optional = true } diff --git a/crates/wasi/src/lib.rs b/crates/wasi/src/lib.rs index 49101a4b02d6..7be134130ea4 100644 --- a/crates/wasi/src/lib.rs +++ b/crates/wasi/src/lib.rs @@ -84,3 +84,45 @@ pub mod snapshots { } } } + +/// Exit the process with a conventional OS error code as long as Wasmtime +/// understands the error. If the error is not an `I32Exit` or `Trap`, return +/// the error back to the caller for it to decide what to do. +/// +/// Note: this function is designed for CLI usage of Wasmtime; embedders of +/// Wasmtime may want different error handling. +pub fn maybe_exit_on_error(e: anyhow::Error) -> anyhow::Error { + use std::process; + use wasmtime::Trap; + + // If a specific WASI error code was requested then that's + // forwarded through to the process here without printing any + // extra error information. + if let Some(exit) = e.downcast_ref::() { + // Print the error message in the usual way. + // On Windows, exit status 3 indicates an abort (see below), + // so return 1 indicating a non-zero status to avoid ambiguity. + if cfg!(windows) && exit.0 >= 3 { + process::exit(1); + } + process::exit(exit.0); + } + + // If the program exited because of a trap, return an error code + // to the outside environment indicating a more severe problem + // than a simple failure. + if e.is::() { + eprintln!("Error: {:?}", e); + + if cfg!(unix) { + // On Unix, return the error code of an abort. + process::exit(128 + libc::SIGABRT); + } else if cfg!(windows) { + // On Windows, return 3. + // https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/abort?view=vs-2019 + process::exit(3); + } + } + + e +} diff --git a/src/commands/run.rs b/src/commands/run.rs index 8468756f3fbd..1ea33fe9f820 100644 --- a/src/commands/run.rs +++ b/src/commands/run.rs @@ -3,18 +3,15 @@ use anyhow::{anyhow, bail, Context as _, Result}; use clap::Parser; use once_cell::sync::Lazy; +use std::ffi::OsStr; +use std::path::{Component, Path, PathBuf}; use std::sync::Arc; use std::thread; use std::time::Duration; -use std::{ - ffi::OsStr, - path::{Component, Path, PathBuf}, - process, -}; -use wasmtime::{Engine, Func, Linker, Module, Store, Trap, Val, ValType}; +use wasmtime::{Engine, Func, Linker, Module, Store, Val, ValType}; use wasmtime_cli_flags::{CommonOptions, WasiModules}; +use wasmtime_wasi::maybe_exit_on_error; use wasmtime_wasi::sync::{ambient_authority, Dir, TcpListener, WasiCtxBuilder}; -use wasmtime_wasi::I32Exit; #[cfg(feature = "wasi-nn")] use wasmtime_wasi_nn::WasiNnCtx; @@ -222,38 +219,10 @@ impl RunCommand { { Ok(()) => (), Err(e) => { - // If a specific WASI error code was requested then that's - // forwarded through to the process here without printing any - // extra error information. - if let Some(exit) = e.downcast_ref::() { - // Print the error message in the usual way. - // On Windows, exit status 3 indicates an abort (see below), - // so return 1 indicating a non-zero status to avoid ambiguity. - if cfg!(windows) && exit.0 >= 3 { - process::exit(1); - } - process::exit(exit.0); - } - - // If the program exited because of a trap, return an error code - // to the outside environment indicating a more severe problem - // than a simple failure. - if e.is::() { - eprintln!("Error: {:?}", e); - - if cfg!(unix) { - // On Unix, return the error code of an abort. - process::exit(128 + libc::SIGABRT); - } else if cfg!(windows) { - // On Windows, return 3. - // https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/abort?view=vs-2019 - process::exit(3); - } - } - - // Otherwise fall back on Rust's default error printing/return + // Exit the process if Wasmtime understands the error; + // otherwise, fall back on Rust's default error printing/return // code. - return Err(e); + return Err(maybe_exit_on_error(e)); } }