Stream: git-wasmtime

Topic: wasmtime / PR #11114 generalize async fiber abstraction

Wasmtime GitHub notifications bot (Jun 23 2025 at 21:45):

dicej opened PR #11114 from dicej:generalized-fiber-abstraction to bytecodealliance:main:

As part of the work implementing the new Component Model async ABI in the wasip3-prototyping repo, I've generalized the FiberFuture abstraction in wasmtime::runtime::store::async_ to support fibers which can either retain exclusive access to the store across suspend points or release it. The latter allows the store to be used by the component-model-async event loop and/or other fibers to run before the original fiber resumes, which is the key to allowing multiple fibers to run concurrently, passing control of the store back and forth.

In the case of Pulley, the above generalization means we also need to give each fiber its own Interpreter so that multiple concurrent fibers don't clobber each other's state.

Concretely, this moves a lot of the code out of async_.rs and into a new fiber.rs submodule which will be shared with the component-model-async implementation.

This also pulls in a new StoreToken<T> utility which has been useful in wasip3-prototyping to safely convert from a &mut dyn VMStore to a StoreContextMut<'a, T> when we previously witnessed a conversion in the other direction.

Note that I've added a 'static bound to the VMStore trait, which simplifies use of &mut dyn VMStore, avoiding thorny lifetime issues.

<!--
Please make sure you include the following information:

• If this work has been discussed elsewhere, please include a link to that
  conversation. If it was discussed in an issue, just mention "issue #...".

• Explain why this change is needed. If the details are in an issue already,
  this can be brief.

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

Wasmtime GitHub notifications bot (Jun 23 2025 at 21:45):

dicej requested alexcrichton for a review on PR #11114.

Wasmtime GitHub notifications bot (Jun 23 2025 at 21:45):

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

Wasmtime GitHub notifications bot (Jun 23 2025 at 21:45):

dicej requested wasmtime-default-reviewers for a review on PR #11114.

Wasmtime GitHub notifications bot (Jun 23 2025 at 21:52):

dicej updated PR #11114.

Wasmtime GitHub notifications bot (Jun 24 2025 at 16:45):

alexcrichton submitted PR review:

I'll keep taking a look at fiber.rs, but this is at least an initial round of feedback.

Wasmtime GitHub notifications bot (Jun 24 2025 at 16:45):

alexcrichton created PR review comment:

Mind prefixing this with SAFETY:?

Wasmtime GitHub notifications bot (Jun 24 2025 at 16:45):

alexcrichton created PR review comment:

I was surprised how much 'static bounds cropped up all over the place from this refactoring and this is what I ended up tracing it back to. This 'static can be replaced with 'a and with that I believe many transitive callers of this function can also remove 'static

<details>

<summary>diff</summary>

diff --git a/crates/wasmtime/src/runtime/component/func/typed.rs b/crates/wasmtime/src/runtime/component/func/typed.rs
index f87447af7..c378742da 100644
--- a/crates/wasmtime/src/runtime/component/func/typed.rs
+++ b/crates/wasmtime/src/runtime/component/func/typed.rs
@@ -179,7 +179,7 @@ where
     ) -> Result<Return>
     where
         Params: Send + Sync,
-        Return: Send + Sync + 'static,
+        Return: Send + Sync,
     {
         let mut store = store.as_context_mut();
         assert!(
diff --git a/crates/wasmtime/src/runtime/fiber.rs b/crates/wasmtime/src/runtime/fiber.rs
index ba34e9e6b..76a89b3e7 100644
--- a/crates/wasmtime/src/runtime/fiber.rs
+++ b/crates/wasmtime/src/runtime/fiber.rs
@@ -585,7 +585,7 @@ unsafe fn suspend_fiber(

 /// Run the specified function on a newly-created fiber and `.await` its
 /// completion.
-pub(crate) async fn on_fiber<R: Send + 'static>(
+pub(crate) async fn on_fiber<R: Send>(
     store: &mut StoreOpaque,
     func: impl FnOnce(&mut StoreOpaque) -> R + Send,
 ) -> Result<R> {
@@ -596,7 +596,7 @@ pub(crate) async fn on_fiber<R: Send + 'static>(
 }

 /// Wrap the specified function in a fiber and return it.
-fn prepare_fiber<'a, R: Send + 'static>(
+fn prepare_fiber<'a, R: Send + 'a>(
     store: &mut dyn VMStore,
     func: impl FnOnce(&mut dyn VMStore) -> R + Send + 'a,
 ) -> Result<(StoreFiber<'a>, oneshot::Receiver<R>)> {
@@ -612,7 +612,7 @@ fn prepare_fiber<'a, R: Send + 'static>(

 /// Run the specified function on a newly-created fiber and `.await` its
 /// completion.
-async fn on_fiber_raw<R: Send + 'static>(
+async fn on_fiber_raw<R: Send>(
     store: &mut StoreOpaque,
     func: impl FnOnce(&mut dyn VMStore) -> R + Send,
 ) -> Result<R> {
diff --git a/crates/wasmtime/src/runtime/func/typed.rs b/crates/wasmtime/src/runtime/func/typed.rs
index e9304b755..0df0dcc40 100644
--- a/crates/wasmtime/src/runtime/func/typed.rs
+++ b/crates/wasmtime/src/runtime/func/typed.rs
@@ -132,10 +132,7 @@ where
         &self,
         mut store: impl AsContextMut<Data: Send>,
         params: Params,
-    ) -> Result<Results>
-    where
-        Results: 'static,
-    {
+    ) -> Result<Results> {
         let mut store = store.as_context_mut();
         assert!(
             store.0.async_support(),
diff --git a/crates/wasmtime/src/runtime/store/async_.rs b/crates/wasmtime/src/runtime/store/async_.rs
index c34d1dac4..f1c8a7454 100644
--- a/crates/wasmtime/src/runtime/store/async_.rs
+++ b/crates/wasmtime/src/runtime/store/async_.rs
@@ -171,7 +171,7 @@ impl StoreOpaque {
     /// This function will convert the synchronous `func` into an asynchronous
     /// future. This is done by running `func` in a fiber on a separate native
     /// stack which can be suspended and resumed from.
-    pub(crate) async fn on_fiber<R: Send + 'static>(
+    pub(crate) async fn on_fiber<R: Send>(
         &mut self,
         func: impl FnOnce(&mut Self) -> R + Send,
     ) -> Result<R> {
@@ -286,7 +286,7 @@ impl StoreOpaque {

 impl<T> StoreContextMut<'_, T> {
     /// Executes a synchronous computation `func` asynchronously on a new fiber.
-    pub(crate) async fn on_fiber<R: Send + 'static>(
+    pub(crate) async fn on_fiber<R: Send>(
         &mut self,
         func: impl FnOnce(&mut StoreContextMut<'_, T>) -> R + Send,
     ) -> Result<R>

</summary>

would that be possible to avoid the extra 'static cropping up? I can sort of see how component-model-async bits may still require 'static, but for preexisting bits in Wasmtime I'd be surprised if it required 'static.

Wasmtime GitHub notifications bot (Jun 24 2025 at 16:45):

alexcrichton created PR review comment:

Mind adding some brief comments as to the purpose of this type?

Wasmtime GitHub notifications bot (Jun 24 2025 at 16:45):

alexcrichton created PR review comment:

Could this be directly imported at use-sites below to avoid the #[cfg]'d import?

Wasmtime GitHub notifications bot (Jun 24 2025 at 16:45):

alexcrichton created PR review comment:

This runs a risk of causing problems because the async feature previously I believe didn't require the std feature, and now it does. Would it be possible to refactor to remove the need for the oneshot bits?

Wasmtime GitHub notifications bot (Jun 24 2025 at 16:45):

alexcrichton created PR review comment:

Would it be possible for this to return &mut fiber::AsyncState instead of a raw pointer?

Wasmtime GitHub notifications bot (Jun 24 2025 at 16:45):

alexcrichton created PR review comment:

FWIW this style of imports is currently largely nonexistent within Wasmtime. Would you be ok reformatting this in a style more consistent with the rest of Wasmtime?

Wasmtime GitHub notifications bot (Jun 24 2025 at 16:45):

alexcrichton created PR review comment:

If possible I think it'd be good to do the same to the Send bounds as well, but I haven't gamed that out yet.

Wasmtime GitHub notifications bot (Jun 24 2025 at 16:45):

alexcrichton created PR review comment:

I'm not sure how exactly yet, but I'd prefer to avoid this #[cfg] necessity just to access self.executor as it's very easy to forget an non-obvious that it's required.

Wasmtime GitHub notifications bot (Jun 24 2025 at 16:45):

alexcrichton created PR review comment:

In retrospect let's add 'static here. There's weird things with variance about extending or shortening lifetimes in types (e.g. being able to safely change a StoreToken<T> to a StoreToken<U> where just the lifetimes differ). Having 'static means we wouldn't have to worry about it though.

Wasmtime GitHub notifications bot (Jun 24 2025 at 16:45):

alexcrichton created PR review comment:

Where possible I'd like to aggressively remove pub(crate) from this file. This function for example in wasip3-prototyping is only used elsewhere in Instance::resume_fiber and the implementation looks like a good candidate for moving to this file as a method on StoreFiber<'static>, so could this function be made purely internal?

Wasmtime GitHub notifications bot (Jun 24 2025 at 16:45):

alexcrichton created PR review comment:

This to me feels like it's a safety contract that's a bit too onerous to uphold since none of the callers of this function actually have a &mut StoreOpaque or similar in context. You'd have to travel a layer or two up the stack to actually see that and it feels a bit too "far" to me to find the proof of "ah, yes, ok this is safe". Would it be possible to actually take &mut StoreOpaque here as an argument? (that would also simplify the executor handling pieces I believe)

Wasmtime GitHub notifications bot (Jun 24 2025 at 16:45):

alexcrichton created PR review comment:

To expand on this some more, this is a pretty unsafe function and the boundaries of abstraction aren't really that clear to me. Understanding this function in isolation is pretty difficult and I feel it fits best within the context of on_fiber_raw or Instance::resume_fiber, which is where I'd prefer to keep all these unsafe set/reset/restore/etc bits all centralized in one function where possible so it's clear what's responsible for what.

Wasmtime GitHub notifications bot (Jun 24 2025 at 16:45):

alexcrichton created PR review comment:

One of the sticklers for taking an argument is probably Drop for StoreFiber, and that's something where we can soup up the Drop for Store to call a function over here to avoid relying on Drop for StoreFiber (or something like that)

Wasmtime GitHub notifications bot (Jun 24 2025 at 16:45):

alexcrichton created PR review comment:

Well, on second thought, no maybe that's not a good idea. Fibers can't be dropped unless they've finished so that would introduce footguns of forgetting to call the destructor on panics for example (or something like that)...

If the dtor is a "special case" that seems somewhat reasonable

Wasmtime GitHub notifications bot (Jun 24 2025 at 16:45):

alexcrichton created PR review comment:

Actually, on raeding this more, it looks like Instance::resume_fiber and on_fiber_raw above are more-or-less the same function where the only difference is Instance::resume_fiber can have a different type of return value which on_fiber_raw would assert never happens. Could the methods all get refactored/merged into this file?

Wasmtime GitHub notifications bot (Jun 24 2025 at 17:03):

dicej submitted PR review.

Wasmtime GitHub notifications bot (Jun 24 2025 at 17:03):

dicej created PR review comment:

I _think_ you can use oneshot without enabling the futures/std feature, so we might be able to just remove this line. I'll play around with it.

Wasmtime GitHub notifications bot (Jun 24 2025 at 17:04):

dicej created PR review comment:

I believe we did this to satisfy miri, but I can retest with &mut and see what happens.

Wasmtime GitHub notifications bot (Jun 24 2025 at 17:04):

dicej submitted PR review.

Wasmtime GitHub notifications bot (Jun 24 2025 at 17:38):

dicej updated PR #11114.

Wasmtime GitHub notifications bot (Jun 24 2025 at 17:39):

dicej submitted PR review.

Wasmtime GitHub notifications bot (Jun 24 2025 at 17:39):

dicej created PR review comment:

I've applied your patch to remove the 'static bounds; let me know if you want me to do the same for Send.

Wasmtime GitHub notifications bot (Jun 24 2025 at 17:47):

alexcrichton created PR review comment:

Yeah if possible I think we should remove Send here too as I think 'static and Send are the root of https://github.com/bytecodealliance/wasip3-prototyping/issues/145 perhaps?

Wasmtime GitHub notifications bot (Jun 24 2025 at 17:47):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jun 24 2025 at 17:50):

dicej created PR review comment:

Yeah, miri won't tolerate that:

test scenario::round_trip::async_round_trip_stackful ... error: Undefined Behavior: attempting a read access using <3440249> at alloc1440939[0x18], but that tag does not exist in the borrow stack for this location
    --> /home/dicej/p/wasmtime/crates/wasmtime/src/runtime/fiber.rs:444:57
     |
444  |         let _reset_executor = Reset(fiber.executor_ptr, *fiber.executor_ptr);
     |                                                         ^^^^^^^^^^^^^^^^^^^
     |                                                         |
     |                                                         attempting a read access using <3440249> at alloc1440939[0x18], but that tag does not exist in the borrow stack for this location
     |                                                         this error occurs as part of an access at alloc1440939[0x18..0x20]
     |
     = help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental
     = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information
help: <3440249> was created by a SharedReadWrite retag at offsets [0x18..0x20]
    --> /home/dicej/p/wasmtime/crates/wasmtime/src/runtime/fiber.rs:476:18
     |
476  |         unsafe { &raw mut (*store.store_opaque_mut().async_state()).current_executor };
     |                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
help: <3440249> was later invalidated at offsets [0x0..0x38] by a Unique retag
    --> /home/dicej/p/wasmtime/crates/wasmtime/src/runtime/store.rs:1947:9
     |
1947 |         &mut self.async_state
     |         ^^^^^^^^^^^^^^^^^^^^^

We might be able to address it a different way (e.g. a larger refactoring), though. Not sure how deep that rabbit hole will go.

Wasmtime GitHub notifications bot (Jun 24 2025 at 17:50):

dicej submitted PR review.

Wasmtime GitHub notifications bot (Jun 24 2025 at 18:15):

dicej submitted PR review.

Wasmtime GitHub notifications bot (Jun 24 2025 at 18:15):

dicej created PR review comment:

Oof, and it looks like miri is upset for other reasons on wasip3-prototyping main; I seem to have caused a regression with the fiber unification PR. Debugging now.

Wasmtime GitHub notifications bot (Jun 24 2025 at 18:24):

dicej updated PR #11114.

Wasmtime GitHub notifications bot (Jun 24 2025 at 18:25):

dicej submitted PR review.

Wasmtime GitHub notifications bot (Jun 24 2025 at 18:25):

dicej created PR review comment:

Okay, just pushed a fix for that regression: https://github.com/bytecodealliance/wasmtime/pull/11114/commits/ae227b658643761ebb3603a428879a826e4f0762

Wasmtime GitHub notifications bot (Jun 24 2025 at 18:50):

dicej updated PR #11114.

Wasmtime GitHub notifications bot (Jun 24 2025 at 20:59):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jun 24 2025 at 20:59):

alexcrichton created PR review comment:

Oh I forgot to mention this earlier but FWIW this looks like it's copy/pasted from elsewhere and would be good to deduplicate.

Wasmtime GitHub notifications bot (Jun 24 2025 at 20:59):

alexcrichton created PR review comment:

Toying around a bit with wasip3-prototyping, for this to be more sound we're going to want to add Send + Sync to the bounds of fun. That makes the unsafe impl Send/Sync above a bit more sound because it at least accounts for all captured values within fun. The fallout wasn't too large from what I saw and seemed mostly as-expected

Wasmtime GitHub notifications bot (Jun 24 2025 at 22:30):

dicej updated PR #11114.

Wasmtime GitHub notifications bot (Jun 24 2025 at 22:32):

dicej created PR review comment:

We chatted about this over Zoom and concluded that changing the *mut to a &mut would require a larger-scale refactoring to make sound and miri-approved, so we're leaving it as-is for now.

Wasmtime GitHub notifications bot (Jun 24 2025 at 22:32):

dicej submitted PR review.

Wasmtime GitHub notifications bot (Jun 24 2025 at 22:33):

dicej submitted PR review.

Wasmtime GitHub notifications bot (Jun 24 2025 at 22:33):

dicej created PR review comment:

I've pushed an update which addresses this.

Wasmtime GitHub notifications bot (Jun 24 2025 at 22:49):

dicej updated PR #11114.

Wasmtime GitHub notifications bot (Jun 24 2025 at 23:33):

dicej updated PR #11114.

Wasmtime GitHub notifications bot (Jun 25 2025 at 16:19):

alexcrichton updated PR #11114.

Wasmtime GitHub notifications bot (Jun 25 2025 at 16:44):

alexcrichton updated PR #11114.

Wasmtime GitHub notifications bot (Jun 25 2025 at 16:46):

dicej updated PR #11114.

Wasmtime GitHub notifications bot (Jun 25 2025 at 21:48):

dicej updated PR #11114.

Wasmtime GitHub notifications bot (Jun 25 2025 at 22:15):

dicej updated PR #11114.

Wasmtime GitHub notifications bot (Jun 25 2025 at 22:50):

dicej updated PR #11114.

Wasmtime GitHub notifications bot (Jun 26 2025 at 16:45):

alexcrichton updated PR #11114.

Wasmtime GitHub notifications bot (Jun 26 2025 at 16:57):

alexcrichton updated PR #11114.

Wasmtime GitHub notifications bot (Jun 26 2025 at 17:58):

alexcrichton updated PR #11114.

Wasmtime GitHub notifications bot (Jun 26 2025 at 19:02):

alexcrichton updated PR #11114.

Wasmtime GitHub notifications bot (Jun 26 2025 at 19:13):

alexcrichton updated PR #11114.

Wasmtime GitHub notifications bot (Jun 26 2025 at 19:16):

alexcrichton updated PR #11114.

Wasmtime GitHub notifications bot (Jun 26 2025 at 19:31):

alexcrichton updated PR #11114.

Wasmtime GitHub notifications bot (Jun 26 2025 at 19:33):

alexcrichton submitted PR review:

@dicej ok these are all the changes I'd like to make, mind reviewing the changes I made as well before merging? Afterwards feel free to go ahead and queue this for merge

Wasmtime GitHub notifications bot (Jun 26 2025 at 20:54):

dicej submitted PR review.

Wasmtime GitHub notifications bot (Jun 26 2025 at 20:54):

dicej created PR review comment:

This comment is confusing since it refers to nonexistent docs for resume_fiber, and in any case we don't need to handwave about the "contract" anymore since resume_fiber now takes an explicit &mut StoreOpaque parameter. So maybe just say something like "This fiber will only be resumed using resume_fiber, which takes a &mut StoreOpaque parameter and has given us exclusive access...".

Wasmtime GitHub notifications bot (Jun 26 2025 at 20:59):

dicej updated PR #11114.

Wasmtime GitHub notifications bot (Jun 26 2025 at 20:59):

dicej has enabled auto merge for PR #11114.

Wasmtime GitHub notifications bot (Jun 26 2025 at 21:38):

dicej updated PR #11114.

Wasmtime GitHub notifications bot (Jun 26 2025 at 21:38):

dicej has enabled auto merge for PR #11114.

Wasmtime GitHub notifications bot (Jun 26 2025 at 22:15):

dicej merged PR #11114.

Last updated: Dec 06 2025 at 07:03 UTC