Skip to content

fix panic when dropping waitable set with stackful waiter#12971

Merged
alexcrichton merged 1 commit intobytecodealliance:mainfrom
dicej:drop-waitable-set-with-stackful-waiters
Apr 6, 2026
Merged

fix panic when dropping waitable set with stackful waiter#12971
alexcrichton merged 1 commit intobytecodealliance:mainfrom
dicej:drop-waitable-set-with-stackful-waiters

Conversation

@dicej
Copy link
Copy Markdown
Contributor

@dicej dicej commented Apr 6, 2026

In waitable_set_drop, we must check for any waiters on the set and, if there are any present, trap without removing the set. Otherwise, if one or more of those waiters are stackful (i.e. WaitMode::Fiber(_)), then removing the set will cause StoreFiber::drop to be called, which will panic since the fiber is still running.

By trapping without removing the set, we defer dropping the fiber(s) until the Store is disposed, at which point we will dispose of them gracefully prior to dropping them.

@dicej dicej requested a review from alexcrichton April 6, 2026 17:35
@dicej dicej requested a review from a team as a code owner April 6, 2026 17:35
@@ -0,0 +1,135 @@
;;! component_model_async = true
;;! component_model_async_stackful = true
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can this be rewritten to not use *_stackful = true?

I think that should be fairly easy, just using a callback option that only has an unreachable instruction

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I don't believe that's possible (or at least, I can't think of a way). The problem is that the test relies on being able to re-enter the instance while another task is already running on that instance, but that's not allowed if the already-running task is stackless (i.e. callback-based).

I just verified that by adding callbacks to component $C's two exports and got a "deadlock detected: event loop cannot make further progress" trap instead of the desired "cannot drop waitable set with waiters" trap.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

For the record, I imagine we could do this by enabling component_model_threading instead component_model_stackful and spawning a thread in the task, but I don't think that's any better or more understandable than what we have now.

In `waitable_set_drop`, we must check for any waiters on the set and, if there
are any present, trap without removing the set.  Otherwise, if one or more of
those waiters are stackful (i.e. `WaitMode::Fiber(_)`), then removing the set
will cause `StoreFiber::drop` to be called, which will panic since the fiber is
still running.

By trapping without removing the set, we defer dropping the fiber(s) until the
`Store` is disposed, at which point we will dispose of them gracefully prior to
dropping them.
@dicej dicej force-pushed the drop-waitable-set-with-stackful-waiters branch from 6ff9712 to 51ea1af Compare April 6, 2026 20:32
@alexcrichton alexcrichton enabled auto-merge April 6, 2026 20:35
@alexcrichton alexcrichton added this pull request to the merge queue Apr 6, 2026
Merged via the queue into bytecodealliance:main with commit 5247390 Apr 6, 2026
48 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants