wasmtime / PR #5484 wasi-threads: an initial implementation · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #5484 wasi-threads: an initial implementation

Wasmtime GitHub notifications bot (Dec 20 2022 at 23:49):

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.

Wasmtime GitHub notifications bot (Dec 20 2022 at 23:50):

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.

Wasmtime GitHub notifications bot (Jan 03 2023 at 16:24):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jan 03 2023 at 16:24):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jan 03 2023 at 16:24):

alexcrichton created PR review comment:

It's ok to leave this file out

Wasmtime GitHub notifications bot (Jan 03 2023 at 16:24):

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 the get_cx callback through to spawn so the host argument could be projected into a &WasiThreadsCtx<T>

Wasmtime GitHub notifications bot (Jan 03 2023 at 16:24):

alexcrichton created PR review comment:

Could you file an issue and list a TODO here for "we shouldn't serialize calls to random_get"?

Wasmtime GitHub notifications bot (Jan 03 2023 at 16:24):

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 the panic! below also needs better handling I think)

Wasmtime GitHub notifications bot (Jan 03 2023 at 16:24):

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.

Wasmtime GitHub notifications bot (Jan 03 2023 at 16:24):

alexcrichton created PR review comment:

If possible I think we're omitting LICENSE files except for the one at the top of the repo

Wasmtime GitHub notifications bot (Jan 06 2023 at 23:46):

abrown updated PR #5484 from wasi-threads-revisited to main.

Wasmtime GitHub notifications bot (Jan 06 2023 at 23:53):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Jan 06 2023 at 23:53):

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.

Wasmtime GitHub notifications bot (Jan 07 2023 at 00:00):

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 what run.rs does for traps and I think we would want to do something similar here--essentially call std::process::exit. The problem is that calling std::process::exit is likely not what one wants to do in an embedding scenario. I would suggest we do implement wasi-threads to initially std::process::exit and then open an issue to think about this for the embedding scenario: I suspect users here would want a trap or proc_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.

Wasmtime GitHub notifications bot (Jan 07 2023 at 00:00):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Jan 09 2023 at 14:50):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jan 09 2023 at 14:50):

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?

Wasmtime GitHub notifications bot (Jan 09 2023 at 20:03):

abrown updated PR #5484 from wasi-threads-revisited to main.

Wasmtime GitHub notifications bot (Jan 09 2023 at 20:16):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Jan 09 2023 at 20:16):

abrown created PR review comment:

Ok, to resolve this I rebased again on main and applied 2ba9f11.

Wasmtime GitHub notifications bot (Jan 27 2023 at 01:43):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Jan 27 2023 at 01:43):

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?

Wasmtime GitHub notifications bot (Jan 27 2023 at 01:44):

abrown updated PR #5484 from wasi-threads-revisited to main.

Wasmtime GitHub notifications bot (Jan 27 2023 at 02:00):

abrown updated PR #5484 from wasi-threads-revisited to main.

Wasmtime GitHub notifications bot (Jan 27 2023 at 02:03):

abrown updated PR #5484 from wasi-threads-revisited to main.

Wasmtime GitHub notifications bot (Jan 27 2023 at 03:43):

abrown updated PR #5484 from wasi-threads-revisited to main.

Wasmtime GitHub notifications bot (Jan 27 2023 at 03:58):

abrown updated PR #5484 from wasi-threads-revisited to main.

Wasmtime GitHub notifications bot (Jan 27 2023 at 18:39):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jan 27 2023 at 18:39):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jan 27 2023 at 18:39):

alexcrichton created PR review comment:

It's ok to not add this since it's implicit from the dependency

Wasmtime GitHub notifications bot (Jan 27 2023 at 18:39):

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.

Wasmtime GitHub notifications bot (Jan 27 2023 at 18:39):

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 and Linker<T>. Could you either add a TODO to perform such type-checking here or implement it in this PR?

Wasmtime GitHub notifications bot (Jan 27 2023 at 18:39):

alexcrichton created PR review comment:

Like with files I think this can all go away now

Wasmtime GitHub notifications bot (Jan 27 2023 at 18:39):

alexcrichton created PR review comment:

Could you add a TODO here that the Mutex 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.

Wasmtime GitHub notifications bot (Jan 27 2023 at 18:39):

alexcrichton created PR review comment:

Similar to above I think many of these changes can go away now too

Wasmtime GitHub notifications bot (Jan 27 2023 at 18:39):

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.

Wasmtime GitHub notifications bot (Jan 30 2023 at 23:18):

abrown updated PR #5484 from wasi-threads-revisited to main.

Wasmtime GitHub notifications bot (Jan 31 2023 at 01:15):

abrown updated PR #5484 from wasi-threads-revisited to main.

Wasmtime GitHub notifications bot (Jan 31 2023 at 01:32):

abrown updated PR #5484 from wasi-threads-revisited to main.

Wasmtime GitHub notifications bot (Jan 31 2023 at 21:39):

abrown updated PR #5484 from wasi-threads-revisited to main.

Wasmtime GitHub notifications bot (Jan 31 2023 at 21:41):

abrown updated PR #5484 from wasi-threads-revisited to main.

Wasmtime GitHub notifications bot (Jan 31 2023 at 22:39):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jan 31 2023 at 22:39):

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)

Wasmtime GitHub notifications bot (Jan 31 2023 at 22:39):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jan 31 2023 at 22:39):

alexcrichton created PR review comment:

Could this perhaps s/Error/thread panicked/ to clearly indicate a panic happen?

Wasmtime GitHub notifications bot (Feb 01 2023 at 01:36):

abrown updated PR #5484 from wasi-threads-revisited to main.

Wasmtime GitHub notifications bot (Feb 01 2023 at 03:24):

abrown updated PR #5484 from wasi-threads-revisited to main.

Wasmtime GitHub notifications bot (Feb 01 2023 at 15:40):

alexcrichton updated PR #5484 from wasi-threads-revisited to main.

Wasmtime GitHub notifications bot (Feb 01 2023 at 16:35):

abrown has marked PR #5484 as ready for review.

Wasmtime GitHub notifications bot (Feb 01 2023 at 16:38):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Feb 01 2023 at 16:38):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Feb 01 2023 at 16:38):

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);

Wasmtime GitHub notifications bot (Feb 01 2023 at 16:38):

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 the std::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 the get_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)

Wasmtime GitHub notifications bot (Feb 01 2023 at 17:31):

abrown updated PR #5484 from wasi-threads-revisited to main.

Wasmtime GitHub notifications bot (Feb 01 2023 at 18:29):

abrown updated PR #5484 from wasi-threads-revisited to main.

Wasmtime GitHub notifications bot (Feb 01 2023 at 19:31):

abrown updated PR #5484 from wasi-threads-revisited to main.

Wasmtime GitHub notifications bot (Feb 01 2023 at 23:03):

abrown updated PR #5484 from wasi-threads-revisited to main.

Wasmtime GitHub notifications bot (Feb 01 2023 at 23:27):

pchickey submitted PR review.

Wasmtime GitHub notifications bot (Feb 01 2023 at 23:27):

pchickey created PR review comment:

bikeshed, but I think NOTSUP is a more appropriate error code for this
            Err(Error::not_supported())

Wasmtime GitHub notifications bot (Feb 01 2023 at 23:38):

pchickey submitted PR review.

Wasmtime GitHub notifications bot (Feb 01 2023 at 23:38):

pchickey created PR review comment:

Same

Wasmtime GitHub notifications bot (Feb 01 2023 at 23:38):

pchickey submitted PR review.

Wasmtime GitHub notifications bot (Feb 01 2023 at 23:38):

pchickey created PR review comment:

Could we return an error instead of panic if for some reason this lock is poisoned?

Wasmtime GitHub notifications bot (Feb 01 2023 at 23:38):

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

Wasmtime GitHub notifications bot (Feb 01 2023 at 23:38):

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?

Wasmtime GitHub notifications bot (Feb 02 2023 at 00:42):

abrown updated PR #5484 from wasi-threads-revisited to main.

Wasmtime GitHub notifications bot (Feb 02 2023 at 00:43):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Feb 02 2023 at 00:43):

abrown created PR review comment:

Ended up doing this anyways in this PR to try to get CI to go green.

Wasmtime GitHub notifications bot (Feb 02 2023 at 00:56):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Feb 02 2023 at 00:56):

abrown created PR review comment:

Sure, but which: perm? invalid_argument? not_supported? I don't know if any fit well.

Wasmtime GitHub notifications bot (Feb 02 2023 at 00:59):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Feb 02 2023 at 00:59):

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?

Wasmtime GitHub notifications bot (Feb 02 2023 at 01:19):

abrown updated PR #5484 from wasi-threads-revisited to main.

Wasmtime GitHub notifications bot (Feb 02 2023 at 01:22):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Feb 02 2023 at 01:22):

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.

Wasmtime GitHub notifications bot (Feb 02 2023 at 01:42):

pchickey submitted PR review.

Wasmtime GitHub notifications bot (Feb 02 2023 at 01:42):

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.

Wasmtime GitHub notifications bot (Feb 02 2023 at 01:49):

pchickey submitted PR review.

Wasmtime GitHub notifications bot (Feb 07 2023 at 19:01):

tschneidereit submitted PR review.

Wasmtime GitHub notifications bot (Feb 07 2023 at 20:10):

abrown updated PR #5484 from wasi-threads-revisited to main.

Wasmtime GitHub notifications bot (Feb 07 2023 at 21:43):

abrown merged PR #5484.

Wasmtime GitHub notifications bot (Feb 09 2023 at 09:45):

yamt submitted PR review.

Wasmtime GitHub notifications bot (Feb 09 2023 at 09:45):

yamt submitted PR review.

Wasmtime GitHub notifications bot (Feb 09 2023 at 09:45):

yamt created PR review comment:

if i read this correctly, this wait would always return 1 immediately without sleeping.

Wasmtime GitHub notifications bot (Feb 09 2023 at 09:45):

yamt created PR review comment:

unused local

Wasmtime GitHub notifications bot (Feb 09 2023 at 09:45):

yamt created PR review comment:

this function doesn't seem thread-safe.

Wasmtime GitHub notifications bot (Feb 10 2023 at 19:00):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Feb 10 2023 at 19:00):

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...

Wasmtime GitHub notifications bot (Feb 10 2023 at 19:02):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Feb 10 2023 at 19:02):

abrown created PR review comment:

Are you thinking about atomic stores or a lock around the $__wasi_fd_write call? (Recall that wasi-common has some internal locking).

Wasmtime GitHub notifications bot (Feb 11 2023 at 05:27):

yamt submitted PR review.

Wasmtime GitHub notifications bot (Feb 11 2023 at 05:27):

yamt created PR review comment:

no. it's about the linear memory used for iovec.

Wasmtime GitHub notifications bot (Feb 23 2023 at 23:28):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Feb 23 2023 at 23:28):

abrown created PR review comment:

Fixed in #5858

Last updated: Nov 22 2024 at 16:03 UTC