github-actions[bot] commented on issue #5098:
Subscribe to Label Action
cc @peterhuene
<details>
This issue or pull request has been labeled: "wasmtime:api"Thus the following users have been cc'd because of the following labels:
- peterhuene: wasmtime:api
To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.
Learn more.
</details>
alexcrichton commented on issue #5098:
Personally I'm a bit hesitant to dive into this world of errors and downcasting and trying to make
Trap
look like ananyhow::Error
, mostly because we'll probably just have a long tail of feature requests. I wonder, though, if we could perhaps make a slight change to the embedding APIs and solve this? For example I think the underlying issue here is the ability for an embedder to return an error in a host function and get back the original error on "the other side". I think that could be possible with:
- Change all host functions to return
anyhow::Error
as theResult
error instead ofTrap
- Remove the
From
impls forTrap
for error-looking things- Change all Wasmtime APIs that return
Result<_, Trap>
to instead returnanyhow::Result<_>
That would effectively mean that we punt all error handling to
anyhow
and everything is plumbed through there rather than having to go throughTrap
. Traps would then always be a "leaf" error which wouldn't contain anything other than a message -- and we could perhaps even do away with that since there's probably no need for a host-createdTrap
at that point any more.While it would be a more impactful change in terms of embedding APIs at least to me it feels more sustainable over time than slowly moving much of
anyhow::Error
intoTrap
which. Basically, in retrospect, I think that the current design ofTrap
may be a mistake.
pchickey commented on issue #5098:
I spent some time thinking about this and I totally agree we should try changing the design to use anyhow everywhere and see how it goes. For instance, it has the additional knock-on effect that the i32 exit can become a concrete user error
struct I32Exit(i32)
belonging to thewasi-common
crate, something we all wanted since the i32 variant was added to traps when Wasi was created.However, I'm worrying about prioritizing this redesign right now, because it will involve touching a lot of crates, tests, and downstream consumers of wasmtime. This project isn't on the critical path for our current thrust to make the component model a usable thing. So, with that in mind, I'd rather merge this non-breaking (except for the Box error thing) change now, and then circle back and revisit the whole design a bit later.
pchickey commented on issue #5098:
It occurred to me that we only need to merge this PR to get the benefits of moving WASI to a concrete exit type, so I guess thats an additional argument towards merging - it makes our migration path clearer
pchickey edited a comment on issue #5098:
It occurred to me that we only need to merge this PR to get the benefits of moving WASI to a concrete exit type, so I guess thats an additional argument towards merging - it makes our migration path incremental
alexcrichton commented on issue #5098:
I'm ok taking a more expedient route, but I wouldn't expect this to have much "complicated" churn with it since signatures can largely "just" be updated and
?
should continue to work as usual. I realize though that the churn, while easy to handle, can be large and cumbersome.As-is though this'll need to change a little bit since when printing a stack of errors this'll double-print the error that the trap represents with
anyhow::Error
. It'll be printed byDisplay for Trap
and then it will additionally be printed by the.source()
since that returns the same trap. I think that we would at least need to fix that before landing, even in an interim state.
pchickey commented on issue #5098:
It appears to me that the trap source only gets printed twice with
println!("{:?}", anyhow::Error::from(the_trap))
- anyhow's display impl doesn't print the source. I don't think we should make Trap's Display impl less useful when the only benefit is to improve an anyhow::Error wrapper's Debug impl.use anyhow::Result; use wasmtime::*; fn main() -> Result<()> { let mut store = Store::<()>::default(); let wat = r#" (module (func $hello (import "" "hello")) (func (export "run") (call $hello)) ) "#; #[derive(Debug)] struct MyTrap; impl std::fmt::Display for MyTrap { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { write!(f, "my trap") } } impl std::error::Error for MyTrap {} let module = Module::new(store.engine(), wat)?; let hello_type = FuncType::new(None, None); let hello_func = Func::new(&mut store, hello_type, |_, _, _| { Err(Trap::from(anyhow::Error::from(MyTrap))) }); let instance = Instance::new(&mut store, &module, &[hello_func.into()])?; let run_func = instance.get_typed_func::<(), (), _>(&mut store, "run")?; let e = run_func .call(&mut store, ()) .err() .expect("error calling function"); println!("display run_func's err: {}", e); println!("debug run_func's err: {:?}", e); let source = std::error::Error::source(&e).expect("trap has a source"); println!("display err's source: {}", source); println!("debug err's source: {:?}", source); source .downcast_ref::<MyTrap>() .expect("source downcasts to MyTrap"); drop(source); let a = anyhow::Error::from(e); println!("display run_func's anyhow'd err: {}", a); println!("debug run_func's anyhow'd err: {:?}", a); let source = a.source().expect("anyhow trap has a source"); println!("display anyhow'd err's source: {}", source); println!("debug anyhow'd err's source: {:?}", source); Ok(()) }
display run_func's err: my trap wasm backtrace: 0: 0x2c - <unknown>!<wasm function 1> debug run_func's err: Trap { reason: Error(my trap), wasm_trace: [FrameInfo { module_name: None, func_index: 1, func_name: None, func_start: FilePos(43), instr: Some(FilePos(44)), symbols: [] }], runtime_trace: Backtrace([Frame { pc: 140485629587494, fp: 140726870698496 }]) } display err's source: my trap debug err's source: MyTrap display run_func's anyhow'd err: my trap wasm backtrace: 0: 0x2c - <unknown>!<wasm function 1> debug run_func's anyhow'd err: my trap wasm backtrace: 0: 0x2c - <unknown>!<wasm function 1> Caused by: my trap display anyhow'd err's source: my trap debug anyhow'd err's source: MyTrap
pchickey edited a comment on issue #5098:
It appears to me that the trap source only gets printed twice with
println!("{:?}", anyhow::Error::from(the_trap))
- anyhow's display impl doesn't print the source. I don't think we should make Trap's Display impl less useful when the only benefit is to improve an anyhow::Error wrapper's Debug impl.use anyhow::Result; use wasmtime::*; fn main() -> Result<()> { let mut store = Store::<()>::default(); let wat = r#" (module (func $hello (import "" "hello")) (func (export "run") (call $hello)) ) "#; #[derive(Debug)] struct MyTrap; impl std::fmt::Display for MyTrap { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { write!(f, "my trap") } } impl std::error::Error for MyTrap {} let module = Module::new(store.engine(), wat)?; let hello_type = FuncType::new(None, None); let hello_func = Func::new(&mut store, hello_type, |_, _, _| { Err(Trap::from(anyhow::Error::from(MyTrap))) }); let instance = Instance::new(&mut store, &module, &[hello_func.into()])?; let run_func = instance.get_typed_func::<(), (), _>(&mut store, "run")?; let e = run_func .call(&mut store, ()) .err() .expect("error calling function"); println!("display run_func's err: {}", e); println!("debug run_func's err: {:?}", e); let source = std::error::Error::source(&e).expect("trap has a source"); println!("display err's source: {}", source); println!("debug err's source: {:?}", source); source .downcast_ref::<MyTrap>() .expect("source downcasts to MyTrap"); drop(source); let a = anyhow::Error::from(e); println!("display run_func's anyhow'd err: {}", a); println!("debug run_func's anyhow'd err: {:?}", a); let source = a.source().expect("anyhow trap has a source"); println!("display anyhow'd err's source: {}", source); println!("debug anyhow'd err's source: {:?}", source); source .downcast_ref::<MyTrap>() .expect("source downcasts to MyTrap"); Ok(()) }
display run_func's err: my trap wasm backtrace: 0: 0x2c - <unknown>!<wasm function 1> debug run_func's err: Trap { reason: Error(my trap), wasm_trace: [FrameInfo { module_name: None, func_index: 1, func_name: None, func_start: FilePos(43), instr: Some(FilePos(44)), symbols: [] }], runtime_trace: Backtrace([Frame { pc: 140485629587494, fp: 140726870698496 }]) } display err's source: my trap debug err's source: MyTrap display run_func's anyhow'd err: my trap wasm backtrace: 0: 0x2c - <unknown>!<wasm function 1> debug run_func's anyhow'd err: my trap wasm backtrace: 0: 0x2c - <unknown>!<wasm function 1> Caused by: my trap display anyhow'd err's source: my trap debug anyhow'd err's source: MyTrap
alexcrichton commented on issue #5098:
Right yeah the hit is to
Debug for anyhow::Error
, although that's the primary mechanism by which errors are displayed AFAIK foranyhow
, notably used by the CLI for example.If you'd prefer I'd be happy to take on the change to use
anyhow::Error
everywhere and remove the ability forTrap
to contain arbitrary host errors, personally I think that's the best way forward without contorting ourselves too much internally withinwasmtime::Trap
pchickey commented on issue #5098:
OK, that works for me. I took a whack at it the other day for about 90 minutes and got sidetracked into wiggle. I'd rather go mess with snapshot1.wasm today than traps :)
Last updated: Dec 23 2024 at 12:05 UTC