- Notifications
You must be signed in to change notification settings - Fork1.6k
Introduce an OOM-handlingError type for Wasmtime#12163
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
ab7ef38 to1329403Comparefitzgen commentedDec 12, 2025
I am not sure why the version bump followed by https://github.com/bytecodealliance/wasmtime/actions/runs/20176633165/job/57926334414?pr=12163 |
fitzgen commentedDec 12, 2025
Third bullet point over in#12164 |
This new `Error` has an API that is 99% identical to `anyhow::Error`'s API, butadditionally handles memory exhaustion.This commit only introduces the `wasmtime_internal_error` crate into ourworkspace, along with its regular tests and OOM tests. This commit does not,however, migrate Wasmtime's internals or public-facing API over to the new errortype yet. That is left for follow up work.In order to continue fitting `Result<(), Error>` in one word, there is quite abit of unsafe code in `Error`'s implementation, mostly surrounding the manualcreation of our own moral equivalent of `Box<dyn Error>` with explicit vtablesand type erasure so that we get a thin pointer to a trait object rather than`Box<dyn Error>`'s fat pointer. To alleviate the associated risks, I've beentesting this code under MIRI throughout its whole development, as well asthoroughly testing the API so that MIRI can dynamically exercise all the codepaths. Furthermore, I've enabled testing this crate under MIRI in CI.
For compatibility with `anyhow`'s API.This unfortunately means we have to move the actual error implementation outinto a submodule so that this function isn't visible, or else every `match .. {Ok(_) => ... }` stops type checking.fitzgen commentedDec 12, 2025
Rebased on top of that PR now that it merged |
fitzgen commentedDec 12, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Grrrr this bump-and-vet CI failure is really annoying me. DetailsEdit: d'oh I missed a command from CI. Can repro. |
fitzgen commentedDec 12, 2025
Looks likefbac398 did indeed fix the |
Subscribe to Label Actioncc@fitzgen DetailsThis issue or pull request has been labeled: "fuzzing"Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
fitzgen commentedDec 15, 2025
@cfallin do you feel comfortable reviewing this since Alex is out-of-office right now? |
cfallin commentedDec 15, 2025
Sure, happy to do so! |
cfallin left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Partial review but I wanted to checkpoint these comments at least -- will keep reading fromlib.rs onward in the new crate tomorrow!
This is very nice code overall; just little nits mostly.
| use backtrace::Backtrace; | ||
| use std::{alloc::GlobalAlloc, cell::Cell, mem, ptr, time}; | ||
| usewasmtime::{Error,Result}; | ||
| usewasmtime_error::{Error,OutOfMemory,Result, bail}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I assume that we are doing this direct import from the error crate for now since it's not integrated into Wasmtime, but is it the long-term goal to reexport the public error types fromwasmtime itself? If so, should we leave a TODO/FIXME here noting that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yes, I have follow up work to publicly re-exportwasmtime_error aswasmtime::error (and also continue to havewasmtime::Error andwasmtime::Result at the top level for convenience).
But also, the fuzzing crate is internal and will never be published, so it doesn't really matter how it names the types, directly or via re-export. It already, for example, pulls inwasmtime-environ and uses that directly in a few places, rather than constraining itself to only using the canonical publicwasmtime API.
I can just fix it right now in my current WIP branch to actually getwasmtime using this crate instead ofanyhow, rather than add a comment that I immediately remove, if that is okay with you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ah, yes, that would be fine too -- not a big deal either way, just wanted to ensure we don't keep an intermediate state of affairs around forever.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
cfallin left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
And here's a review on the rest of the diff. Thanks for building all of this -- in general it looks quite good! My only real concern is the dangling-pointer issue in prior review. With that addressed (or adequately documented if I'm wrong about the bad case?) I'm happy to r+.
I'll note as well that my review is at the level of "plausible and looks correct"; Alex may still have opinions about what isbest but I suspect that can be left to followup as well.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
… for all `ConcreteError<E>`s that we actually allocate
…layout requirements
fitzgen commentedDec 16, 2025
@cfallin I believe I've addressed all of your feedback, and this should be ready for another look. |
cfallin left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
OK, this looks correct to me and seems reasonable as far as my opinions of error interfaces go!
Uh oh!
There was an error while loading.Please reload this page.
fitzgen commentedDec 16, 2025
Regarding this CI failure:https://github.com/bytecodealliance/wasmtime/actions/runs/20283910974/job/58252868624 I don't think this PR can pass that CI step and merge until@wasmtime-publish is an owner of the |
fitzgen commentedDec 16, 2025
Will try again, just in case that CI step will somehow accept the invite. |
cfallin commentedDec 16, 2025
Seems like an oversight in our procedures --@alexcrichton when you're back, would you be willing to write up a runbook entry for "I added a new crate to Wasmtime" with the way to make the current publish flow happy? (It seems I've gotten caught in similar issues a few times too, whether it be the "published crates" list or deferred problems on next release or ...) |
fitzgen commentedDec 16, 2025
This seems to have made the |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
This new
Errorhas an API that is 99% identical toanyhow::Error's API, but additionally handles memory exhaustion.This commit only introduces the
wasmtime_internal_errorcrate into our workspace, along with its regular tests and OOM tests. This commit does not, however, migrate Wasmtime's internals or public-facing API over to the new error type yet. That is left for follow up work.In order to continue fitting
Result<(), Error>in one word, there is quite a bit of unsafe code inError's implementation, mostly surrounding the manual creation of our own moral equivalent ofBox<dyn Error>with explicit vtables and type erasure so that we get a thin pointer to a trait object rather thanBox<dyn Error>'s fat pointer. To alleviate the associated risks, I've been testing this code under MIRI throughout its whole development, as well as thoroughly testing the API so that MIRI can dynamically exercise all the code paths. Furthermore, I've enabled testing this crate under MIRI in CI.Part of#12069