wasi-threads: check module instantiability during construction #5741
Conversation
Due to bytecodealliance#5675, we could not perform an early check whether the module passed to `wasi-threads` could indeed be instantiated. Now that that issue is resolved, this change performs the instantiation check (via `Linker::instantiate_pre`) during construction.
| if let Err(err) = linker.instantiate_pre(&module) { | ||
| bail!( | ||
| "wasi-threads will not be able to instantiate the passed module: {:?}", | ||
| err | ||
| ); | ||
| } | ||
| Ok(Self { module, linker }) |
There was a problem hiding this comment.
Thinking about this again, the WasiThreadsCtx could actually store InstancePre<T> and use that to instantiate as there's no need to recreate it each time a thread is spawned.
Could ::new be refactored to either:
- Take a
&Moduleand&Linker<T>and persistArc<InstancePre<T>>withinWasiThreadsCtx - Or take a
InstancePre<T>directly and store anArcinternally in the context
| // We have checked during construction that the module can be | ||
| // instantiated and an entry point exists with the correct | ||
| // signature--these calls should not fail. | ||
| let instance = linker.instantiate(&mut store, &module).unwrap(); |
There was a problem hiding this comment.
Thinking about this again, this is fine for now but I think this is still something that the wasi-threads proposal needs to happen. Basically the fallibility of spawning a thread I don't think is handled well. For example the OS could fail to spawn a thread, an embedder may want to limit threads, allocating resources for an Instance may fail (e.g. local linear memories if any). There's a lot of failure modes beyond "the imports didn't line up" so it's possible to trigger this unwrap() I think (although hard for the "CLI world").
Basically I think that the wasi-threads proposal needs a way for an embedder to communicate a thread spawn failure and I think that the failure here needs to be communicated back somehow. That has its own set of thorny questions though because how "blocking" is the thread-spawn intrinsic? E.g. we ideally don't want to instantiate the instance on the caller thread ,but in the child thread like this. That means though that failure would be delayed since the thread may not start for a bit. Anyway I'm rambling now. Fine for now, but I think needs more work on the proposal.
There was a problem hiding this comment.
Yeah, this is a good point. This error should be communicated back to the WebAssembly guest; it isn't always a runtime failure. The wasi-threads proposal currently designates negative numbers as error codes so I think what could be done here is specify some of that range to match some of the possible reasons for failure here. How can I get the full list of possible errors that could be unwrapped here?
There was a problem hiding this comment.
I am not aware of an exhaustive list of errors, but some examples I can think of are:
- The OS thread failed to spawn for an OS-defined reason (e.g. not enough kernel memory, not enough user memory, kernel is limiting threads, etc)
- An embedder-configured limit prevented a thread from being spawned (e.g. your store gets at most 5 threads)
- Module instantiation on the new thread failed (e.g. not enough memory, some store configuration limited growth of table/memory, not enough virtual memory for a second non-shared linear memory, etc)
- Module instantiation could fail due to a trap in the
startfunction
And that's sort of just a subset of what could happen. Not necessarily all of those should be communicated through an error code perhaps. For example maybe failure to instantiate is considered a fatal error that aborts all threads, but perhaps failure to spawn a thread at the OS level is communicated back to the thread-spawn caller.
In any case, though, I think the upstream proposal should have a rough set of guidelines for what to do in these failure situations and how embedders communicate this to wasm.
abrown
changed the title
This change relaxes what kinds of modules can be run when wasi-threads is enabled via `--wasi-modules experimental-wasi-threads`. Previously, as reported in bytecodealliance#6153, simple modules that made no use of thread spawning or shared memories were preemptively rejected when the wasi-threads context was created. This is not ideal because it is too restrictive. Instead, this change does the following: - it moves the check for whether a module is valid according to the wasi-threads specification to the point a new thread is spawned; this resolves bytecodealliance#6153 - as noted in bytecodealliance#6153, this change also adds a better error message indicating that wasi-threads expects a shared memory import - the way this is implemented also improves the module instantiation: by constructing an `InstancePre` once when the `WasiThreadsCtx` is built, this might shave off a bit of time from the "spawn a thread" call; this supercedes bytecodealliance#5741
This change relaxes what kinds of modules can be run when wasi-threads is enabled via `--wasi-modules experimental-wasi-threads`. Previously, as reported in bytecodealliance#6153, simple modules that made no use of thread spawning or shared memories were preemptively rejected when the wasi-threads context was created. This is too restrictive. Instead, this change does the following: - it moves the check for whether a module is valid according to the wasi-threads specification to the point a new thread is spawned; this resolves bytecodealliance#6153 - as noted in bytecodealliance#6153, this change also adds a better error message indicating that wasi-threads expects a shared memory import - the way this is implemented also improves the module instantiation: by constructing an `InstancePre` once when the `WasiThreadsCtx` is built, we might shave off a bit of time from the "spawn a thread" call; this supercedes a similar effort in bytecodealliance#5741
This change relaxes what kinds of modules can be run when wasi-threads is enabled via `--wasi-modules experimental-wasi-threads`. Previously, as reported in #6153, simple modules that made no use of thread spawning or shared memories were preemptively rejected when the wasi-threads context was created. This is too restrictive. Instead, this change does the following: - it moves the check for whether a module is valid according to the wasi-threads specification to the point a new thread is spawned; this resolves #6153 - as noted in #6153, this change also adds a better error message indicating that wasi-threads expects a shared memory import - the way this is implemented also improves the module instantiation: by constructing an `InstancePre` once when the `WasiThreadsCtx` is built, we might shave off a bit of time from the "spawn a thread" call; this supercedes a similar effort in #5741
|
Superseded by #6492. |
This change relaxes what kinds of modules can be run when wasi-threads is enabled via `--wasi-modules experimental-wasi-threads`. Previously, as reported in bytecodealliance#6153, simple modules that made no use of thread spawning or shared memories were preemptively rejected when the wasi-threads context was created. This is too restrictive. Instead, this change does the following: - it moves the check for whether a module is valid according to the wasi-threads specification to the point a new thread is spawned; this resolves bytecodealliance#6153 - as noted in bytecodealliance#6153, this change also adds a better error message indicating that wasi-threads expects a shared memory import - the way this is implemented also improves the module instantiation: by constructing an `InstancePre` once when the `WasiThreadsCtx` is built, we might shave off a bit of time from the "spawn a thread" call; this supercedes a similar effort in bytecodealliance#5741
Due to #5675, we could not perform an early check whether the module passed to
wasi-threadscould indeed be instantiated. Now that that issue is resolved, this change performs the instantiation check (viaLinker::instantiate_pre) during construction.