Stream: git-wasmtime

Topic: wasmtime / PR #5741 wasi-threads: check module instantiat...


view this post on Zulip Wasmtime GitHub notifications bot (Feb 07 2023 at 22:08):

abrown opened PR #5741 from use-5675 to main:

Due to #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.

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

Please ensure all communication adheres to the code of conduct.
-->

view this post on Zulip Wasmtime GitHub notifications bot (Feb 07 2023 at 22:09):

abrown requested alexcrichton for a review on PR #5741.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 07 2023 at 22:35):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 07 2023 at 22:35):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 07 2023 at 22:35):

alexcrichton created PR review 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.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 07 2023 at 22:35):

alexcrichton created PR review 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:

view this post on Zulip Wasmtime GitHub notifications bot (Feb 07 2023 at 23:00):

abrown submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 07 2023 at 23:00):

abrown created PR review 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?

view this post on Zulip Wasmtime GitHub notifications bot (Feb 08 2023 at 14:57):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 08 2023 at 14:57):

alexcrichton created PR review comment:

I am not aware of an exhaustive list of errors, but some examples I can think of are:

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.


Last updated: Dec 23 2024 at 12:05 UTC