Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

wasi-threads: check module instantiability during construction #5741

Closed
wants to merge 1 commit into from
Closed

wasi-threads: check module instantiability during construction #5741

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 10 additions & 9 deletions crates/wasi-threads/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,12 @@ impl<T: Clone + Send + 'static> WasiThreadsCtx<T> {
WASI_ENTRY_POINT
);
}
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 })
Comment on lines +30 to 36
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 &Module and &Linker<T> and persist Arc<InstancePre<T>> within WasiThreadsCtx
  • Or take a InstancePre<T> directly and store an Arc internally in the context

}

Expand All @@ -44,15 +50,10 @@ impl<T: Clone + Send + 'static> WasiThreadsCtx<T> {
// Each new instance is created in its own store.
let mut store = Store::new(&module.engine(), host);

// Ideally, we would have already checked much earlier (e.g.,
// `new`) whether the module can be instantiated. Because
// `Linker::instantiate_pre` requires a `Store` and that is only
// available now. TODO:
// https://github.com/bytecodealliance/wasmtime/issues/5675.
let instance = linker.instantiate(&mut store, &module).expect(&format!(
"wasi-thread-{} exited unsuccessfully: failed to instantiate",
wasi_thread_id
));
// 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();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 start function

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.

let thread_entry_point = instance
.get_typed_func::<(i32, i32), ()>(&mut store, WASI_ENTRY_POINT)
.unwrap();
Expand Down