Wasmtime GitHub notifications bot (Feb 07 2023 at 22:08): 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-threadscould indeed be instantiated. Now that that issue is resolved, this change performs the instantiation check (viaLinker::instantiate_pre) during construction.<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
Wasmtime GitHub notifications bot (Feb 07 2023 at 22:09):abrown requested alexcrichton for a review on PR #5741.
Wasmtime GitHub notifications bot (Feb 07 2023 at 22:35):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Feb 07 2023 at 22:35):alexcrichton submitted PR review.
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
Instancemay 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 thisunwrap()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.
Wasmtime GitHub notifications bot (Feb 07 2023 at 22:35):alexcrichton created PR review comment:
Thinking about this again, the
WasiThreadsCtxcould actually storeInstancePre<T>and use that to instantiate as there's no need to recreate it each time a thread is spawned.Could
::newbe 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
Wasmtime GitHub notifications bot (Feb 07 2023 at 23:00):abrown submitted PR review.
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-threadsproposal 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?
Wasmtime GitHub notifications bot (Feb 08 2023 at 14:57):alexcrichton submitted PR review.
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:
- 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
startfunctionAnd 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: Oct 23 2024 at 20:03 UTC