DO NOT MERGE: Prototype some potential error handling ways #192

dignifiedquire · 2022-04-06T12:03:36Z

Split by commits

Some utilities to make the code dealing with errors nicer in general
Introduce a Abort error that calls rt::abort when converted into, allowing a very explicit triggering via type conversion on the highlevel methods, while still being able to use ActorError in all internal methods without triggering aborts.

actors/runtime/src/actor_error.rs

Stebalien · 2022-04-06T19:26:48Z

actors/account/src/lib.rs

@@ -34,25 +34,23 @@ pub enum Method {
 pub struct Actor;
 impl Actor {
    /// Constructor for Account actor
-    pub fn constructor<BS, RT>(rt: &mut RT, address: Address) -> Result<(), ActorError>
+    pub fn constructor<BS, RT>(rt: &mut RT, address: Address) -> Result<(), Abort>


Given that Abort can't happen, why not just return ()?

Because I wanted to potential for actually returning an error in debug scenarios. My idea was that we could have sth like a debug feature, where Abort would collect data and actually be a returned value.

One alternative design I had was

fn foo() -> Abort<T> {} type Abort<T> = Result<T, AbortError>;

to make it even more visual that this is a special kind of exit.

Because I wanted to potential for actually returning an error in debug scenarios. My idea was that we could have sth like a debug feature, where Abort would collect data and actually be a returned value.

Ok, that's reasonable. Although, IMO, we get better information if we abort immediately (and ask wasmtime for a backtrace). See https://docs.wasmtime.dev/api/wasmtime/struct.Config.html#method.wasm_backtrace_details.

There's some scope for divergent behaviour between the two cases here. I guess that this Abort is intended to always be immediately propagated with ?.

Up at this high level of the actor entry points, I would probably lean towards Steb's initial take that actually aborting is better. We would want a way to shim that call for testing, though, wrapping out the underlying static call so we can use a mock/fake VM instead.

Stebalien · 2022-04-06T19:36:16Z

I like the usability, but I also kind of hate it because things that look like they should return an error, actually just fail.

How about just adding a panic handler? That way we can add .unwrap() things.

Or add a custom unwrap_or_abort method where we want to abort? That's a lot more explicit.

dignifiedquire · 2022-04-06T19:50:54Z

That way we can add .unwrap() things.

The reason I don't want to use unwrap is that for me that is a sign of "Error" that should never happen unless there is a serious bug. So I would like to not conflate this

dignifiedquire · 2022-04-06T19:51:53Z

Or add a custom unwrap_or_abort method where we want to abort? That's a lot more explicit.

If we can find a shorter name I would be okay with an explicit call, but I am not fully convinced it is actually better, but I understand your confliction.

Stebalien · 2022-04-06T20:10:28Z

If we can find a shorter name I would be okay with an explicit call, but I am not fully convinced it is actually better, but I understand your confliction.

.yeet()

But actually, we can't do that because that's what we're calling async sends in future FVM versions. Decisions have been made.

dignifiedquire · 2022-04-06T20:59:57Z

I guess we are sticking with ? then :P

anorth · 2022-04-07T00:06:38Z

actors/account/src/lib.rs

@@ -34,25 +34,23 @@ pub enum Method {
 pub struct Actor;
 impl Actor {
    /// Constructor for Account actor
-    pub fn constructor<BS, RT>(rt: &mut RT, address: Address) -> Result<(), ActorError>
+    pub fn constructor<BS, RT>(rt: &mut RT, address: Address) -> Result<(), Abort>


There's some scope for divergent behaviour between the two cases here. I guess that this Abort is intended to always be immediately propagated with ?.

Up at this high level of the actor entry points, I would probably lean towards Steb's initial take that actually aborting is better. We would want a way to shim that call for testing, though, wrapping out the underlying static call so we can use a mock/fake VM instead.

anorth · 2022-04-07T00:08:26Z

actors/account/src/lib.rs

@@ -82,7 +80,7 @@ impl ActorCode for Actor {
                let addr = Self::pubkey_address(rt)?;
                Ok(RawBytes::serialize(addr)?)
            }
-            None => Err(actor_error!(SysErrInvalidMethod; "Invalid method")),
+            None => Err(actor_error!(SysErrInvalidMethod; "Invalid method").into()),


I get this is just prototyping, but I want only one of the ActorError or Abort types to survive. They're going after the same problem.

They definitely should survive both in this construction. The conversion into an Abort is what triggers the abort, where as ActorError still can be used inside an actor to communicate recoverable errors.

I don't think ActorError should be used for recoverable errors. I don't think there are use cases for that. I would say recoverable errors should not specify an abort code.

where would that mapping to codes live then?

anorth · 2022-04-07T00:12:36Z

actors/runtime/src/actor_error.rs

+    #[cfg(not(feature = "fil-actor"))]
+    fn maybe_abort(exit_code: ExitCode, msg: Option<&str>) -> ! {
+        // TODO: maybe not panic, needs discussion what we want here
+        panic!("Abort: {}: {:?}", exit_code, msg);


It works well in Go. If we can reliably catch the panic, e.g. in test harnesses, and inspect the exit code, then this could be good. My read of the Rust docs suggested this was even more frowned upon than in Go, though. cc @ZenGround0

I would make the msg non-optional.

anorth · 2022-04-07T00:13:53Z

actors/runtime/src/actor_error.rs

+macro_rules! ensure {
+    ($cond:expr, $code:ident, $msg:literal $(,)?) => {
+        if !$cond {
+            return Err($crate::actor_error!($code, $msg).into());


Curious: why use into rather than just construct the Abort explicitly here?

Because it is not possible to construct an Abort, only be triggering the into implementation. This does not necessarily convert into an Abort, it could just convert into itself or other Error types depending on where it is used.

Co-authored-by: Steven Allen <steven@stebalien.com>

jennijuju · 2022-04-11T14:57:05Z

(This will be a post M1 task - will be a part of the control flow immediate interruption on error_ epic to match go's behaviour

Stebalien reviewed Apr 6, 2022

View reviewed changes

actors/runtime/src/actor_error.rs Outdated Show resolved Hide resolved

Stebalien reviewed Apr 6, 2022

View reviewed changes

anorth reviewed Apr 7, 2022

View reviewed changes

dignifiedquire and others added 3 commits April 7, 2022 20:06

prototype error handling helpers

5a92754

prototype abort error

09c204e

Update actors/runtime/src/actor_error.rs

bf4c523

Co-authored-by: Steven Allen <steven@stebalien.com>

dignifiedquire force-pushed the errors-round1 branch from 9c92adb to bf4c523 Compare April 7, 2022 18:25

arajasek force-pushed the master branch from 200bc05 to 4aecdee Compare February 10, 2023 21:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DO NOT MERGE: Prototype some potential error handling ways #192

DO NOT MERGE: Prototype some potential error handling ways #192

dignifiedquire commented Apr 6, 2022

Stebalien Apr 6, 2022

dignifiedquire Apr 6, 2022

dignifiedquire Apr 6, 2022

Stebalien Apr 6, 2022

anorth Apr 7, 2022

Stebalien commented Apr 6, 2022

dignifiedquire commented Apr 6, 2022

dignifiedquire commented Apr 6, 2022

Stebalien commented Apr 6, 2022

dignifiedquire commented Apr 6, 2022

anorth Apr 7, 2022

anorth Apr 7, 2022

dignifiedquire Apr 7, 2022

anorth Apr 7, 2022

dignifiedquire Apr 7, 2022

anorth Apr 7, 2022

anorth Apr 7, 2022

dignifiedquire Apr 7, 2022

jennijuju commented Apr 11, 2022 •

edited

Loading

DO NOT MERGE: Prototype some potential error handling ways #192

Are you sure you want to change the base?

DO NOT MERGE: Prototype some potential error handling ways #192

Conversation

dignifiedquire commented Apr 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stebalien commented Apr 6, 2022

dignifiedquire commented Apr 6, 2022

dignifiedquire commented Apr 6, 2022

Stebalien commented Apr 6, 2022

dignifiedquire commented Apr 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jennijuju commented Apr 11, 2022 • edited Loading

jennijuju commented Apr 11, 2022 •

edited

Loading