jameysharp opened PR #6464 from jameysharp:epoch-callback-yield
to bytecodealliance:main
:
When an epoch interruption deadline arrives, previously it was possible to yield to the async executor, or to invoke a callback on the wasm stack, but not both. This changes the API to allow callbacks to run and then request yielding to the async executor.
We discussed this in today's Wasmtime bi-weekly call and I believe this PR reflects the consensus from that discussion. But I'm open to bikeshedding the API.
I was tempted to revise the out-of-gas/fuel API to match the epoch interruption API as well, but decided not to do that in this PR.
jameysharp requested wasmtime-core-reviewers for a review on PR #6464.
jameysharp requested pchickey for a review on PR #6464.
alexcrichton submitted PR review:
Mind adding some tests as well?
alexcrichton submitted PR review:
Mind adding some tests as well?
alexcrichton created PR review comment:
From an ergonomic perspective could this remain
Result<DeadlineResult>
? (maybe with a bit of renaming)That'll allow
?
to be used for fallible operations and matches how traps are handled elsewhere in the embedding API where it's theErr
of aResult
alexcrichton created PR review comment:
Can the documentation for this method be updated to talk about how returning
Yield
will panic ifasync_support
is disabled?
alexcrichton created PR review comment:
Is there any particular reason as to why? If something traps I could see it still being possible resuming other execution in the
Store
which may be unrelated which could require the same callback.
alexcrichton created PR review comment:
In this case I believe that
async_yield_impl
will panic internally if it's not configured right but technically that method's panic should be allowable to be adebug_assert!
or anunwrap_unchecked
or something like that. I think it'd be good to have a dedicated assert here for the use case that this covers whereYield
is returned but async isn't enabled or configured.
alexcrichton requested alexcrichton for a review on PR #6464.
jameysharp created PR review comment:
As far as I could tell this was the existing behavior, but it seemed non-obvious so I added a comment. I don't think there's a strong argument for behaving this way and I agree that it could be useful to preserve the callback.
fitzgen created PR review comment:
Maybe
pub enum DeadlineBehavior {
to make
Result<DeadlineBehavior>
?Another option is to have this return a
std::ops::ControlFlow
where the break is ananyhow::Error
and the continue is either a sync or async enum with au64
payload. This would also allow for?
usage. But my experience withControlFlow
has not been super great.
fitzgen created PR review comment:
To be clear, this would remove the
DeadlineResult::Trap
variant right? Since that would become theResult::Err
?
fitzgen created PR review comment:
Once something in a store traps, you really aren't supposed to use the store again.
alexcrichton created PR review comment:
Yeah that's what I'm thinking
alexcrichton created PR review comment:
Ah ok if this is preserving the existing behavior that's ok, but otherwise while you're here I think it might be good to "fix" this and put the callback back in place regardless of the result.
I agree that most of the time stores/instances/etc should be considered "poisoned" after trapping but that feels like a higher level concern which wouldn't affect a lower-level detail like this. Mostly in that we don't have anything official per-se about nullifying a store when it traps.
jameysharp updated PR #6464.
jameysharp requested wasmtime-default-reviewers for a review on PR #6464.
jameysharp updated PR #6464.
alexcrichton submitted PR review.
alexcrichton has enabled auto merge for PR #6464.
jameysharp updated PR #6464.
jameysharp has enabled auto merge for PR #6464.
jameysharp merged PR #6464.
Last updated: Dec 23 2024 at 12:05 UTC