abrown opened PR #5484 from wasi-threads-revisited
to main
:
This PR builds on several other under-review PRs to demonstrate a working wasi-threads implementation in Wasmtime. The first three commits are cherry-picked from the following PRs:
- #5326
- #5471
- #5475
The last two commits build on these to expose the wasi-threads API to WebAssembly modules. Upon building and running Wasmtime with the right flags, one can then run multi-threaded WebAssembly programs:
$ cargo run --features wasi-threads -- \ --wasm-features threads --wasi-modules experimental-wasi-threads \ <a threads-enabled module>
Due to Cargo feature issues in 05020d4, this PR is not yet expected to compile but, perhaps more interestingly, one can use these changes to run some simple pthread tests over in the wasi-libc repository: https://github.com/WebAssembly/wasi-libc/pull/369.
abrown edited PR #5484 from wasi-threads-revisited
to main
:
This PR builds on several other under-review PRs to demonstrate a working wasi-threads implementation in Wasmtime. The first three commits are cherry-picked from the following PRs:
- #5326
- #5471
- #5475
The last two commits build on these to expose the wasi-threads API to WebAssembly modules. Upon building and running Wasmtime with the right flags, one can then run multi-threaded WebAssembly programs:
$ cargo run --features wasi-threads -- \ --wasm-features threads --wasi-modules experimental-wasi-threads \ <a threads-enabled module>
Due to Cargo feature issues in 05020d4, this PR is not yet expected to pass all CI tasks but, perhaps more interestingly, one can use these changes to run some simple pthread tests over in the wasi-libc repository: https://github.com/WebAssembly/wasi-libc/pull/369.
alexcrichton submitted PR review.
alexcrichton submitted PR review.
alexcrichton created PR review comment:
It's ok to leave this file out
alexcrichton created PR review comment:
If possible I think this would be best avoided because a
Linker<T>
is a costly thing to clone.One option is to wrap it up in an
Arc
, and another would be to thread theget_cx
callback through tospawn
so thehost
argument could be projected into a&WasiThreadsCtx<T>
alexcrichton created PR review comment:
Could you file an issue and list a TODO here for "we shouldn't serialize calls to
random_get
"?
alexcrichton created PR review comment:
I feel like this is a little tricky. Panicking on errors here doesn't feel like the right thing to do since these panics can theoreteically get triggered. In that sense I think that the errors need to be handled here. As you've probably noticed, however, there's nowhere to communicate the error to. One option would be to move the initialization here to the thread calling spawn, but that feels sort of wrong since it sort of belongs in the child thread.
Overall I think this might be best left as an issue on the
wasi-threads
repository. Basically how would a thread spawn operation get notified that the thread was spawned but something else failed? Or otherwise how are traps handled and/or other fatal errors in the threads proposal? (that is thepanic!
below also needs better handling I think)
alexcrichton created PR review comment:
Technically this can be done today, but I think this might be best done as some sort of "join" API from the host which isn't currently specified for
wasi-threads
, so while wait/notify could be added here there's no way to actually wait until all the child threads have exited.
alexcrichton created PR review comment:
If possible I think we're omitting LICENSE files except for the one at the top of the repo
abrown updated PR #5484 from wasi-threads-revisited
to main
.
abrown submitted PR review.
abrown created PR review comment:
It felt strange to create an issue for yet-to-be-merged code; I will do this as a follow-up.
abrown created PR review comment:
Yeah, I've looked into this more since you reviewed this. There has been some discussion in a wasi-threads issue which boils down to "any
proc_exit
or trap in any thread exits the program." I was looking at whatrun.rs
does for traps and I think we would want to do something similar here--essentially callstd::process::exit
. The problem is that callingstd::process::exit
is likely not what one wants to do in an embedding scenario. I would suggest we do implement wasi-threads to initiallystd::process::exit
and then open an issue to think about this for the embedding scenario: I suspect users here would want a trap orproc_exit
to kill all the threads but not kill the original program calling Wasmtime but, with no great way to cancel threads in Rust, I think this might be kind of difficult.
abrown submitted PR review.
alexcrichton submitted PR review.
alexcrichton created PR review comment:
I'm ok with this all being a TODO and not necessarily covered here. At the very least this PR is a good step forward to being able to spawn threads.
I do think these issues need to be resolved before stabilization, but perhaps it's best to track these sorts of things in the wasi-threads repository?
abrown updated PR #5484 from wasi-threads-revisited
to main
.
abrown submitted PR review.
abrown created PR review comment:
Ok, to resolve this I rebased again on
main
and applied 2ba9f11.
abrown submitted PR review.
abrown created PR review comment:
Looking at this again, I think I can rewrite this with a "join" loop after the spawn calls: each thread will atomically increment a memory location and notify the main thread; the main thread waits in a loop and only exits once the read memory equals the number of threads we are waiting for. This fixup could come after this PR merges?
abrown updated PR #5484 from wasi-threads-revisited
to main
.
abrown updated PR #5484 from wasi-threads-revisited
to main
.
abrown updated PR #5484 from wasi-threads-revisited
to main
.
abrown updated PR #5484 from wasi-threads-revisited
to main
.
abrown updated PR #5484 from wasi-threads-revisited
to main
.
alexcrichton submitted PR review.
alexcrichton submitted PR review.
alexcrichton created PR review comment:
It's ok to not add this since it's implicit from the dependency
alexcrichton created PR review comment:
Oh I had forgotten to mention this earlier, but now that other bits and pieces are cleaned up I think most of these changes should no longer be necessary? I think the
BorrowedFile
type should be able to be removed now along with all other changes necessary for this.
alexcrichton created PR review comment:
There are two unwraps below during the
spawn
operation's closure:
- Using
Linker::instantiate
- The call to
get_typed_func
Both of these, however, can be type-checked ahead of time at this location I believe by inspecting
Module
andLinker<T>
. Could you either add a TODO to perform such type-checking here or implement it in this PR?
alexcrichton created PR review comment:
Like with files I think this can all go away now
alexcrichton created PR review comment:
Could you add a
TODO
here that theMutex
here shouldn't be necessary? Something along the lines that this forces threads to serialize on their acquisition of randomness but there's need for such serialization.
alexcrichton created PR review comment:
Similar to above I think many of these changes can go away now too
alexcrichton created PR review comment:
One possibility that isn't caught by this is the
.call(..)
function panicking, such as trying to use wasi-nn in a threaded context. If a panic happens that leaves all other threads alive and running and just this thread exits. For that it may be worthwhile, for now at least, to catch panics here as well and abort the whole process if one is caught.
abrown updated PR #5484 from wasi-threads-revisited
to main
.
abrown updated PR #5484 from wasi-threads-revisited
to main
.
abrown updated PR #5484 from wasi-threads-revisited
to main
.
abrown updated PR #5484 from wasi-threads-revisited
to main
.
abrown updated PR #5484 from wasi-threads-revisited
to main
.
alexcrichton submitted PR review.
alexcrichton created PR review comment:
I think this type can be dropped as well? (a cursory look and I'm not seeing anywhere else it's used at least)
alexcrichton submitted PR review.
alexcrichton created PR review comment:
Could this perhaps
s/Error/thread panicked/
to clearly indicate a panic happen?
abrown updated PR #5484 from wasi-threads-revisited
to main
.
abrown updated PR #5484 from wasi-threads-revisited
to main
.
alexcrichton updated PR #5484 from wasi-threads-revisited
to main
.
abrown has marked PR #5484 as ready for review.
alexcrichton submitted PR review.
alexcrichton submitted PR review.
alexcrichton created PR review comment:
Could this be
module
with something like this in the body of the function:// silence unused warning as it's only used with wasi-threads drop(&module);
alexcrichton created PR review comment:
Could the
Arc
type be imported into this module? (it's used in a number of places and should avoid thestd::sync::...
prefix)Additionally can you leave some comments here for what's going on? Basically something along the lines of how
wasi-crypto
the implementation is not ready for threads yet so theget_mut
implementation here will succeed only if a thread hasn't been spawned. If a thread is spawned, however, this will panic and indicate to the user that work needs to happen to integrate wasi-crypto and threads.(and then a comment on
wasi-nn
below referring to this comment as well)
abrown updated PR #5484 from wasi-threads-revisited
to main
.
abrown updated PR #5484 from wasi-threads-revisited
to main
.
abrown updated PR #5484 from wasi-threads-revisited
to main
.
abrown updated PR #5484 from wasi-threads-revisited
to main
.
pchickey submitted PR review.
pchickey created PR review comment:
bikeshed, but I think NOTSUP is a more appropriate error code for this
Err(Error::not_supported())
pchickey submitted PR review.
pchickey created PR review comment:
Same
pchickey submitted PR review.
pchickey created PR review comment:
Could we return an error instead of panic if for some reason this lock is poisoned?
pchickey created PR review comment:
Terminating the parent process is a really heavy hammer and makes this whole crate unsuitable for use in multi-tenant embeddings. It would be helpful to understand exactly why this crate had to make that design choice, and to please note it somewhere high level, like in a crate-level README
pchickey created PR review comment:
I'd reword "CLI usage" to something like its "for usage where it is acceptable for wasmtime failures to terminate the parent process, such as in the wasmtime CLI", and say something like "not suitable for use in multi-tenant embeddings" as a more obvious warning? Could we hide the presence of this function behind an off-by-default cargo feature, to give more assurance to multi-tenant embeddings that they are safe from this?
abrown updated PR #5484 from wasi-threads-revisited
to main
.
abrown submitted PR review.
abrown created PR review comment:
Ended up doing this anyways in this PR to try to get CI to go green.
abrown submitted PR review.
abrown created PR review comment:
Sure, but which:
perm
?invalid_argument
?not_supported
? I don't know if any fit well.
abrown submitted PR review.
abrown created PR review comment:
Also, if the
Mutex
ever does get poisoned (which doesn't seem likely to happen?) wouldn't we want to crash Wasmtime to figure out what is going on rather than letting the Wasm module proceed?
abrown updated PR #5484 from wasi-threads-revisited
to main
.
abrown submitted PR review.
abrown created PR review comment:
Hopefully this hammer is only temporary; @alexcrichton and I had discussed some early thoughts on how we might shut down all of the threads when necessary. It is complex to do so, though, so providing experimental wasi-threads access sooner for some users seemed more valuable than solving the problem and waiting longer.
pchickey submitted PR review.
pchickey created PR review comment:
Yeah, nevermind, I guess I didn't think this through - this is a per-ctx mutex so it would only get poisoned if another thread panicked already, so panicking more in the same store is fine.
pchickey submitted PR review.
tschneidereit submitted PR review.
abrown updated PR #5484 from wasi-threads-revisited
to main
.
abrown merged PR #5484.
yamt submitted PR review.
yamt submitted PR review.
yamt created PR review comment:
if i read this correctly, this
wait
would always return 1 immediately without sleeping.
yamt created PR review comment:
unused local
yamt created PR review comment:
this function doesn't seem thread-safe.
abrown submitted PR review.
abrown created PR review comment:
Ah, yeah, this is a mistake due to a refactoring: originally I intended the
i
local (unused now) to keep track of the expected values but that was lost as I changed things. I'll add a note to fix this...
abrown submitted PR review.
abrown created PR review comment:
Are you thinking about atomic stores or a lock around the
$__wasi_fd_write
call? (Recall thatwasi-common
has some internal locking).
yamt submitted PR review.
yamt created PR review comment:
no. it's about the linear memory used for iovec.
abrown submitted PR review.
abrown created PR review comment:
Fixed in #5858
Last updated: Dec 23 2024 at 13:07 UTC