wasmtime / PR #14146 align multithreading and trap behavi... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #14146 align multithreading and trap behavi...

Wasmtime GitHub notifications bot (Aug 17 2026 at 15:48):

dicej requested alexcrichton for a review on PR #14146.

Wasmtime GitHub notifications bot (Aug 17 2026 at 15:48):

dicej opened PR #14146 from dicej:spec-threading-and-sync-blocking-updates to bytecodealliance:main:

This updates Wasmtime's Component Model async and cooperative multithreading support to match the current specification, including:

Refined rules for trapping when a sync-typed function blocks. We now enforce this "lazily" rather than "eagerly", mwaning a sync-typed function is allowed to call an async-typed function or blocking intrinsic, and if it doesn't actually block, we won't trap. And if the call _does_ block, we will look for any eligible threads to run and run them until no such threads remain, only trapping if and when we still need to block and have no more threads to run.

Ensure that the predicate for determining which threads can be run when a sync-typed function is executing in an instance matches the spec.

Remove the previous "may block" bookkeeping at the task and root instance level, replacing it with (sub-)instance level tracking of whether any sync-typed function is running in that instance.

Run the event loop during start function calls since they are now allowed to call async-typed functions, create and resume threads, etc.

Note that this includes test/component-model submodule updates which haven't yet been merged to the main branch of the upstream repo, but should be merged soon. See https://github.com/WebAssembly/component-model/pull/696

Fixes #14117

Wasmtime GitHub notifications bot (Aug 17 2026 at 16:03):

dicej updated PR #14146.

Wasmtime GitHub notifications bot (Aug 17 2026 at 17:14):

dicej updated PR #14146.

Wasmtime GitHub notifications bot (Aug 17 2026 at 17:22):

dicej updated PR #14146.

Wasmtime GitHub notifications bot (Aug 18 2026 at 16:44):

:memo: alexcrichton submitted PR review:

I'd like to review more of concurrent.rs but here's some initial thoughts. It's at the point where whenever I expand context on github it just sends me randomly elsewhere in this diff and I keep losing my spot in the otherwise big diff in concurrent.rs. I'm hoping my changes in https://github.com/dicej/wasmtime/pull/7 which reduce the number of files changed helps with that...

Wasmtime GitHub notifications bot (Aug 18 2026 at 16:44):

:speech_balloon: alexcrichton created PR review comment:

Can you update this comment while you're here. Notably this comment is intended to be safety-bearing and it's now inaccurate because new_started isn't used and instead it's split up here depending on how things are run.

Wasmtime GitHub notifications bot (Aug 18 2026 at 16:44):

:speech_balloon: alexcrichton created PR review comment:

Stylistically I find this quite hard to read because everything is indented so far over and spread out across so many lines due to rustfmt's formatting. Could this method be split up with some helpers perhaps to reduce indentation? Either that or using some local variables which project out some state to avoid having to re-acquire fields through projection each time?

Wasmtime GitHub notifications bot (Aug 18 2026 at 16:44):

:speech_balloon: alexcrichton created PR review comment:

Could new_raw return a tuple where one result is "needs start" to avoid duplicating the logic necessary to deduce this?

Wasmtime GitHub notifications bot (Aug 18 2026 at 16:44):

:speech_balloon: alexcrichton created PR review comment:

Personally I still find it valuable to not have this change in the API. I know I'm somewhat swimming upstream but I continue to not want to get into the habit of just readily adding constraints everywhere necessary.

I've sent a PR-to-this-branch as https://github.com/dicej/wasmtime/pull/7 which removes the need for all of the Send changes in this PR. I believe that it's safe to have that PR due to the conditional nature of Send in async and the surrounding context.

Wasmtime GitHub notifications bot (Aug 18 2026 at 16:44):

:speech_balloon: alexcrichton created PR review comment:

To avoid duplicating code, could this be modeled as:
#[cfg(component-model-async)]
if asyncness != No && store.concurrency_support() {
    // do the concurrent thing
} else {
    // use `start_raw`
}

#[cfg(not(component-model-async))]
// use start_raw
Similar to elsewhere I'm pretty worried about the extreme rightward drift here as it makes it pretty hard to understand what's actually going on.

Another possible alternative would be to take the body of the concurrent bits here and move them to concurrent.rs as a dedicated function.

Wasmtime GitHub notifications bot (Aug 18 2026 at 16:44):

:speech_balloon: alexcrichton created PR review comment:

While you're here could you update this comment? Notably there's no longer a split with *_async and the comment about the fiber is no longer correct now that it's natively async

Wasmtime GitHub notifications bot (Aug 18 2026 at 16:44):

:speech_balloon: alexcrichton created PR review comment:

Personally I'm pretty wary of adding new hooks like this because we have so many other locations where this is applicable but not otherwise called. For example don't all of these locations need to in theory call this to update the internal flag?

{Host,FACT}-invoking cabi_realloc

{Host,FACT}-invoking resource destructors

FACT component-to-component trampolines

In all of those locations it's effectively sync but the internal do_not_suspend flag isn't updated I think?

Basically I think it's best if we avoid adding state where possible because there's so many places to maintain the state that it's best to lean on preexisting stae if we can. Could we look at the current task or otherwise have some accessor for what the suspending task's type is and/or something like that?

Wasmtime GitHub notifications bot (Aug 18 2026 at 17:13):

:memo: dicej submitted PR review.

Wasmtime GitHub notifications bot (Aug 18 2026 at 17:13):

:speech_balloon: dicej created PR review comment:

{Host,FACT}-invoking cabi_realloc

cabi_realloc can't call any imports and thus can't suspend, correct?

{Host,FACT}-invoking resource destructors

Yeah, I'll need to look into this.

FACT component-to-component trampolines

I actually did a bunch of work for this (e.g. reverting may_leave back to a bitset which has two flags and propagating that change everywhere) and then reverted it all when I realized that the lazy task/thread creation in StoreOpaque::force_deferred_current_thread covered it. Did I miss something?

Could we look at the current task or otherwise have some accessor for what the suspending task's type is and/or something like that?

That's essentially what I did if you look at the places this function is called: query the task for its type and then publish that to the InstanceState so that other concurrent tasks and threads for that instance can see it. Alternatively, other tasks and threads could find out whether they're able to suspend by iterating over all known tasks in the store's table, looking for any that match the instance of interest and checking their type. That would be pretty expensive, though.

Wasmtime GitHub notifications bot (Aug 18 2026 at 22:38):

dicej updated PR #14146.

Wasmtime GitHub notifications bot (Aug 18 2026 at 23:48):

github-actions[bot] added the label wasmtime:api on PR #14146.

Wasmtime GitHub notifications bot (Aug 19 2026 at 14:33):

:thumbs_up: alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Aug 19 2026 at 14:33):

:speech_balloon: alexcrichton created PR review comment:

Could you expand on what's going on here in a comment?

Wasmtime GitHub notifications bot (Aug 19 2026 at 14:33):

:speech_balloon: alexcrichton created PR review comment:

Could this be re-worded a bit? It seems like false can only be returned for Promote as an argument but otherwise this as-is looks worrisome because it's using bail_bug! for a guest-specified input which seems like it should be a trap. I think this probably all works out regardless, so I think it's just the message here that needs tweaking.

Wasmtime GitHub notifications bot (Aug 19 2026 at 14:33):

:speech_balloon: alexcrichton created PR review comment:

Did I miss something?

Ah ok I see what you mean how this is lazily invoked from the forcing of the current therad. Ok let's leave this as-is and assume that those hooks are sufficiently called. I'm mostly just worried that we have so many hooks and we've historically had a lot of bugs around "forgot to call the hook"

Wasmtime GitHub notifications bot (Aug 19 2026 at 14:34):

alexcrichton commented on PR #14146:

I think this may also still have lingering management of may_block as it looks like there's some here too

Wasmtime GitHub notifications bot (Aug 26 2026 at 18:43):

dicej updated PR #14146.

Wasmtime GitHub notifications bot (Aug 28 2026 at 14:02):

dicej updated PR #14146.

Wasmtime GitHub notifications bot (Aug 28 2026 at 14:06):

dicej updated PR #14146.

Wasmtime GitHub notifications bot (Aug 28 2026 at 14:09):

dicej updated PR #14146.

Wasmtime GitHub notifications bot (Aug 28 2026 at 15:37):

dicej updated PR #14146.

Wasmtime GitHub notifications bot (Aug 28 2026 at 17:51):

dicej updated PR #14146.

Wasmtime GitHub notifications bot (Aug 28 2026 at 19:15):

dicej updated PR #14146.

Wasmtime GitHub notifications bot (Aug 28 2026 at 19:32):

dicej updated PR #14146.

Wasmtime GitHub notifications bot (Aug 28 2026 at 19:32):

dicej requested alexcrichton for a review on PR #14146.

Wasmtime GitHub notifications bot (Aug 28 2026 at 19:33):

dicej commented on PR #14146:

@alexcrichton would you mind taking one more look at this? Aligning it with https://github.com/WebAssembly/component-model/pull/705 required an overhaul and uncovered a few subtle divergences with the spec which I had to address.

Wasmtime GitHub notifications bot (Aug 28 2026 at 19:42):

dicej updated PR #14146.

Wasmtime GitHub notifications bot (Aug 28 2026 at 19:42):

dicej has marked PR #14146 as ready for review.

Wasmtime GitHub notifications bot (Aug 28 2026 at 19:42):

dicej requested wasmtime-compiler-reviewers for a review on PR #14146.

Wasmtime GitHub notifications bot (Aug 28 2026 at 19:42):

dicej requested wasmtime-core-reviewers for a review on PR #14146.

Wasmtime GitHub notifications bot (Aug 28 2026 at 20:47):

:thumbs_up: alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Aug 28 2026 at 20:47):

:speech_balloon: alexcrichton created PR review comment:

We've got to be quite careful about adding new storage of fibers since if this is ever accidentally dropped it means the process will likely terminate. Additionally this needs to hook into things like GC and such.

I see that trace_fiber_roots wasn't updated with this branch, although take_fibers_and_futures was, for example. Could both of those locations switch to exhaustive matches to avoid forgetting this in the future?

Wasmtime GitHub notifications bot (Aug 28 2026 at 20:47):

:speech_balloon: alexcrichton created PR review comment:

I don't think this change is quite right since if this is canceled at the low-priority await point and ready is Some then this'll drop ready without properly disposing of it

Wasmtime GitHub notifications bot (Aug 28 2026 at 20:47):

:speech_balloon: alexcrichton created PR review comment:

This feels pretty bad as it's a linear search over everything in the concurrent table. Could this be managed with a counter of some kind instead or something like that?

Wasmtime GitHub notifications bot (Aug 28 2026 at 20:47):

:speech_balloon: alexcrichton created PR review comment:

An observation on this: if this is actually hit it'll destroy the process due to one of the fields in the .. being a fiber that can't be dropped.

That being said this is actually present in a whole bunch of places throughout this file. In a myriad of locations where there's a fiber in scope there are instances of ? used, which only trigger on bugs, but if they were to occur would also destroy the fiber. I'm not entirely sure what to do about that, but we may want to track that as a general issue of something to improve at some point.

Wasmtime GitHub notifications bot (Aug 28 2026 at 20:47):

:speech_balloon: alexcrichton created PR review comment:

I think both of these branches boil down to set_switch_item, right?

Wasmtime GitHub notifications bot (Aug 28 2026 at 20:47):

:speech_balloon: alexcrichton created PR review comment:

(unresolving this because I think this may still be applicable?)

Wasmtime GitHub notifications bot (Aug 28 2026 at 20:47):

:speech_balloon: alexcrichton created PR review comment:

I know I've said this before, but if you're for helping out at some point in the future this file really could benefit from being split up. It's kind of unreviewable in github's UI because if context is expanded anywhere then the UI jumps to some random position in the diff-of-all-files, meaning you can't actually keep your place in this file. That's almost surely a UI bug on github's side, but I think this file would benefit from being split up for a number of other reasons too.

Wasmtime GitHub notifications bot (Aug 28 2026 at 20:47):

:speech_balloon: alexcrichton created PR review comment:

todo

Wasmtime GitHub notifications bot (Aug 28 2026 at 22:02):

:memo: dicej submitted PR review.

Wasmtime GitHub notifications bot (Aug 28 2026 at 22:02):

:speech_balloon: dicej created PR review comment:

Another possible alternative would be to take the body of the concurrent bits here and move them to concurrent.rs as a dedicated function.

This is what I ended up doing. Would you like me to try to reduce the indentation as well?

Wasmtime GitHub notifications bot (Aug 28 2026 at 22:04):

:memo: dicej submitted PR review.

Wasmtime GitHub notifications bot (Aug 28 2026 at 22:04):

:speech_balloon: dicej created PR review comment:

Ah, good catch.

Wasmtime GitHub notifications bot (Aug 28 2026 at 22:05):

:memo: dicej submitted PR review.

Wasmtime GitHub notifications bot (Aug 28 2026 at 22:05):

:speech_balloon: dicej created PR review comment:

This is only used when we're about trap, so I figured it isn't performance sensitive. With that in mind, do you still think it should be optimized?

Wasmtime GitHub notifications bot (Aug 28 2026 at 22:05):

:speech_balloon: dicej edited PR review comment.

Wasmtime GitHub notifications bot (Aug 28 2026 at 22:06):

:memo: alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Aug 28 2026 at 22:06):

:speech_balloon: alexcrichton created PR review comment:

Ah ok nevermind, I'll poke at this after this lands

Wasmtime GitHub notifications bot (Aug 28 2026 at 22:07):

:memo: dicej submitted PR review.

Wasmtime GitHub notifications bot (Aug 28 2026 at 22:07):

:speech_balloon: dicej created PR review comment:

One is setting the switch item on the _store_, while the other is setting in on the _task_; i.e. they're two different fields. The latter is used to defer setting the store's switch item until the task either suspends or exits.

Wasmtime GitHub notifications bot (Aug 29 2026 at 00:07):

dicej updated PR #14146.

Last updated: Aug 30 2026 at 09:07 UTC