should we return an error rather than assert! in zome API fns?

[thedavidmeister]

see invoke_commit for an example, first line is assert! but fn signature returns a result

plenty of other things that panic throughout...

[zippy]

To me the distinction is based on whether the error is truly a runtime error that could happen at runtime, in which case it MUST be an error message. A panic! or an assert! would be fine if it's something that would happen only in the development cycle of an app. i.e. the library can panic on something that would tell the app developer that they have screwed up in a way detectable during app dev time, but should never panic in a way that will stop a production container/composer that's holding the app instance.

I can't tell which case this assert! falls under.

[thedavidmeister]

@zippy potentially new information: any panic inside anything that is called from WASM cannot be debugged, all you get is Err(ErrorGeneric("Trap: Trap { kind: Unreachable }")) and backtrace does not work because WASM doesn't have the machinery

[zippy]

Right! That's interesting, I hadn't thought about how that would affect calls out from wasm. I kind of like us keeping the same distinction in mind however, and figuring out our own "panic" style error that would die during dev time for errors like this. Maybe that's what this ticket turns into:

"As an app developer I want fatal errors in my use of api calls (like incorrect number of arguments passed to a commit, etc) to throw early in dev cycle with clear error messages so I can debug them"

[thedavidmeister]

@zippy yeah that's more or less the user story

the context is that i did a quick google and found that directly calling anything that panics in rust is fundamentally not supported in wasm, but of course we can return errors and handle them outside wasm, which should allow debugging like what you're talking about

@ddd-mtl already made a good start on this with the HcApiReturnCode

here's some info on the wasm side of the story:

• https://github.com/rust-lang/rust/issues/46996
• https://github.com/WebAssembly/design/blob/master/Semantics.md#unreachable

my thoughts here are that, even if we don't have a sophisticated alternative, we should never call anything that has the option of panicking in wasm because it's not supported by wasm. even in the case that a panic might be impossible today (there are a few examples where some earlier conditional logic makes the later panic impossible), that doesn't mean it won't become tomorrow's debugging nightmare during some future refactor.

the wasm utils module could give us convenience methods for all this if we find a lot of boilerplate cropping up

[zippy]

Next step is to identify all the places where this might happen.

[ddd-mtl]

I think we should keep some asserts for functions that can be called outside of WASM. Like for testing. Those asserts should be striped out in Release so it won't cause a panic when called by WASM.

[timotree3]

Just tried to do this issue since it was marked as good first issue. I looked for invoke_commit but couldn't find it. I also didn't see any assert!s in invoke_commit_app_entry: https://github.com/holochain/holochain-rust/blob/e32401b4e2fb0ba0cdb2e2fed6a0198c4877b92a/core/src/nucleus/ribosome/api/commit.rs#L24-L76

Should this issue be closed? If not, I'd love some pointers as to an up to date example in the code.

[pospi]

FWIW these kinds of Unreachable crashes are a problem in a lot of Diorama setups due to the way tests have been wired for unhandled promise rejections. Implementors should probably be advised to add a short wait before exiting the JS test runner as otherwise useful debug logs are often missed.

Ping @philipbeadle :)

[thedavidmeister]

CC: @maackle