Expose `wasmtime_runtime::InstantiationError` as a stable wasmtime API

pchickey commented 2 years ago

Feature

Presently, the instantiate family of functions (Linker::instantiate, InstancePre::instantiate, their _async cousins etc) error with an anyhow::Error. To inspect those errors, we can try downcasting to wasmtime_runtime::InstantiationError.

Benefit

Users who need to inspect those errors need to keep their deps of wasmtime and wasmtime_runtime in sync. wasmtime_runtime's API is not designed for stable use. Stabilizing this error API gets rid of a possible runtime dep for users.

The InstantiationError::Limit variant is used when a wasmtime pooling allocator is out of instances. This is the only way that wasmtime crate users can observe this condition. There should be a stable way to observe this.

The InstantiationError::Trap variant contains a wasmtime_runtime::Trap, which is different from a wasmtime::Trap in ways that aren't useful to wasmtime users, and can be confusing.

Implementation

I think it makes sense for the instantiate family of functions to still error with an anyhow::Error, but wasmtime should export types to try downcasting that error to.

Wasmtime should not re-export wasmtime_runtime::InstantiationError because it exposes the wrong sort of Trap. Instead it should map the variants to types which are in the public API.

pub enum InstantiationError {
    Resource(anyhow::Error),
    Link(LinkError),
    Trap(Trap),
    Limit(u32),
}

Resource variant: just return the anyhow::Error here.
Link variant: re-export wasmtime_runtime::LinkError for downcasting to that variant, since this is just a string wrapper.
Trap variant: map the contents to a wasmtime::Trap.
Limit variant: Wasmtime could define a new public type PoolingAllocatorLimit and impl Error on it, and map the InstantiationError::Limit variant to that type.

Alternatives

There are probably other good ideas I haven't thought of here! I am very open to suggestions.

alexcrichton commented 2 years ago

Personally I'm hesitant to expose error-enums because in the limit every function would needs its own error enum. For example InstantiatePre::instantiate can't return an InstantiationError::Link, but Instance::new and Linker::instantiate can. There's also a number of other cases like Func::call which can fail for any number of reasons but we lump them all into anyhow::Error.

That being said though it definitely makes sense to want to work with specific kinds of errors and handle those differently than others. For that though we've relied on downcasting where possible. For example for instantiations that fail because of a trap you should be able to downcast_ref::<wasmtime::Trap>() and get the trap out. I don't think we have downcasts for other errors though.

In that sense I would personally prefer to have specific errors you can downcast to instead of having specific errors returned from each API. Is there a particular error you're thinking of looking for beyond traps?

pchickey commented 2 years ago

I think we are in agreement, and I just wasn't clear in the language I used for the proposal above. The error I need most is the Limit error, but I figured we could cover all four variants pretty easily.

bytecodealliance / wasmtime