Xstate: invoke.onError transition not taken when error is thrown

[tivac]

Description

When using invoke with an onError transition I expected throwing an error (or rejecting a promise) from the invoked thing to trigger the onError transition.

Expected Result

onError gets invoked, no error is reported to the console, and I decide what to do with the thrown error.

Actual Result

Console shows a red error and the onError transition is never taken.

Reproduction

https://codesandbox.io/s/xstate-child-machine-errors-h5csv

Additional context

I asked this question in gitter and @davidkpiano suggested that this should work correctly. @amitnovick had a useful workaround where you can sendParent(error(<invoked-id>, data)) which works but feels very clunky and requires child machines to know the id they were invoked with which is problematic.

[amitnovick]

This is an excellent minimal reproduction of the issue! easily confirmed it.

[Andarist]

It works correctly with invoked promises - https://codesandbox.io/s/xstate-child-machine-errors-2hxlu

As to invoked machines - making it work would require defining an extension to SCXML. In SCXML errors are generally added to internal event queue and there is no builtin mechanism for sharing this with a parent.

I'm also not yet sure if supporting this is a good idea - errors can have different root causes, meanings etc (some are expected - e.g. network failure, some are unexpected - e.g. a typo) and explicit error handling (on per call site basis) should be IMHO preferred.

[amitnovick]

As to invoked machines - making it work would require defining an extension to SCXML. In SCXML errors are generally added to internal event queue and there is no builtin mechanism for sharing this with a parent.

I'm also not yet sure if supporting this is a good idea - errors can have different root causes, meanings etc (some are expected - e.g. network failure, some are unexpected - e.g. a typo) and explicit error handling (on per call site basis) should be IMHO preferred.

Fair points.

Just trying to make sense of this a little more; it seems that:

• onDone is similar in function to SCXML's <finalize>
• onError doesn't exist in SCXML

That said, I refrain to give opinion as to whether XState should strictly adhere to SCXML or not (I barely have a working understanding of SCXML at this time).

[Andarist]

onDone is similar in function to SCXML's

That's correct.

onError doesn't exist in SCXML

That's also correct 😉

That said, I refrain to give opinion as to whether XState should strictly adhere to SCXML or not (I barely have a working understanding of SCXML at this time).

In general XState aims to be compatible with SCXML - however, it doesn't mean that it cannot extend it. In this particular case, I'm not sure if onError is a valid extension (for invoked machines) because it seems preferred for error handling to happen locally (in crashed machine) and only communicate a failure to the outside world IF that is what local error handling decides to do - but that decision is always explicit.

If "uncaught" error would "bubble up" to the parent implicitly we would also have to consider what should happen if it stays unhandled - should we kill all ancestors till the root? When root gets killed in such a way - should we kill all spawned children and propagate it to all "leafs"?

[tivac]

Handling errors at the call sites is annoying duplication but workable, it's what I already have to do using the sendParent(error(<id>)) workaround.

The part that really annoys me is that all my current options require duplication of effort to handle:

1. If I use sendParent("ERROR") I have to essentially define the onError for the parent twice, once as the actual onError and once in an { on : { ERROR } }.

2. Or use the awkward sendParent(error(<child-id>)) format, where the child-id param has to be duplicated all over the child machine and has to match the id field defined in the parent machine.

3. Add a new error state to the child machine with type : "final" and make sure that all possible fatal error paths in the child transition to that state.

If throwing an error/rejecting a promise from the child machine would trigger the onError in the parent like I expected it would, none of those workarounds would be necessary. Not catching the error in the child and letting it propagate to the parent is being explicit, it's explicitly choosing to end the child machine because it hit an unrecoverable error state and let the parent machine know what happened.

I'd be fine with this being an opt-in behavior like autoForward if necessary.

[Andarist]

Not catching the error in the child and letting it propagate to the parent is being explicit, it's explicitly choosing to end the child machine because it hit an unrecoverable error state and let the parent machine know what happened.

IMHO omitting an explicit error handler can hardly be considered as being explicit. Problem with propagating errors to parent automatically is that in many situations the parent machine WON'T really be notified about what happened - it will receive some random error, without real intent (in form of an event) associated with it.

You are kinda proposing automatic error bubbling, similar to what try/catch does in languages - but there are perfectly valid concerns why not every programming language has those constructs and why some prefer explicit error passing. It makes you really handle all errors and makes error tracing way easier. With implicit error bubbling it's super hard for a newcomer to know if the error is being handled or not by looking at the call site - because to really know if it is being handled you need to search through all layers for an error handler.

[tivac]

I opened the issue because @davidkpiano told me in Gitter that my original idea (throwing an error in a child machine should trigger the parent onError) should already work and it was a bug that it didn't.

Seems like you and I are just not going to agree on whether or not this is valuable @Andarist 🙂

[amitnovick]

Seems like you and I are just not going to agree on whether or not this is valuable @Andarist slightly_smiling_face

I believe that @Andarist has been very thoughtful with his reply, in which he points out that "fixing" the behaviour to the expected result may not be the best design choice (for the reasons that he lists, and other reasons).

It would be nice if we could get a clarification from @davidkpiano as to what the onError is intended to do, since I assume this was his decision to add onError as an extension to SCXML.

[Andarist]

Keep also in mind - that I'm just discussing and stating personal opinions. I'm not a native english speaker and my writing might suffer from that - might sound a little bit harsh, too confident etc but that is certainly not my intention.

Just to add a little bit to the discussion - I believe that even if u need to introduce some duplication it might be good because it forces you to be explicit which is really helpful for future readers. It also kinda makes the possible visualization better because the intent being visible closer to the failing call site (so on the same diagram, rather than somewhere else).

I wouldn't also recommend "hacking" the system by using things like sendParent(error(<child-id>)) - I would recommend creating a dedicated event for a particular failure. I believe that only error.execution (runtime errors caused by typos, undefined access etc) should really be handled as "errors" - those are unexpected (and can only be avoided with proper testing). Everything else is not that much of an error - it's just something that happens in the system and is expected to happen.

[tivac]

To be clear, I'm not mad that we disagree on this. We just have very opposing opinions on this and that's totally fine!

I've got a solution that works even if it's a little awkward. It's at least less awkward than forcing every invoke state with a machine that can die to handle onError identically in two different places.

[Baael]

I've found this annoying too. I was expecting same behavior as with promises.
My own awkward solution (draft):

import { Machine, Interpreter } from "xstate"
import { error }                from "xstate/lib/actions"

class CustomInterpreter extends Interpreter {
    spawnMachine(machine, options) {
        try {
            super.spawnMachine(machine, options)
        } catch( errorData ) {
            this.send(error(options.id || machine.id, errorData))
        }
    }
}

const interpret = (machine, options) =>  new CustomInterpreter(machine, options)

codesandbox

Based on original code with magic TODO:

} else if (isMachine(source)) {
    // TODO: try/catch here
    this.spawnMachine(data ? source.withContext(mapContext(data, context, event)) : source, {
        id: id,
        autoForward: autoForward
    });
} else {

[davidkpiano]

Taking a look at Akka docs for actor supervision: https://doc.akka.io/docs/akka/current/general/supervision.html

I would say this falls more under the realm of actor supervision than SCXML (which doesn't define behavior for this, and doesn't care if invoked children fail).

We can make a new "error.<something>" event type to handle this, which is allowed by SCXML:

Therefore applications and platforms may extend the set of errors defined in this specification in two ways. First by adding a suffix to an error name defined in this specification, and second by using 'error.platform' with or without a suffix.

https://www.w3.org/TR/scxml/#ErrorEvents

So child services that throw an error can automatically send the parent an e.g., "error.child" event, which is handled by the parent:

on: {
  "error.child": {
    // handle what happens when an error occurs
    actions: (ctx, event) => {
      event.origin; // child ID
      event.data; // error data
    }
  }
}

Thoughts? This would remove the boilerplate of manually sending an error via sendParent('CUSTOM_ERROR').

[tivac]

@davidkpiano would that event also be issued when an error that would normally trigger onError occurs? My entire use case for wanting this is not wanting to be have to have two separate handlers for errors, so just making sure your proposal covers it.

[Andarist]

We can make a new "error." event type to handle this, which is allowed by SCXML:

Extending is definitely OK. I'm not sure if it's OK to send events from error "namespace" to different SCXML sessions though. Every example of error events only adds them to internal queue, there is no example of adding those to external queue (and sending them to external~ entities would require us to add them to their external queue).

I also can't find any statement explicitly forbidding those events from being sent to other sessions, but would be great to reach for some SCXML consultancy regarding this 😅 .

This is just about naming convention - we can always introduce smth like xstate.error namespace - so ultimately for the feature itself it doesn't matter much and can be decided later.

I would say this falls more under the realm of actor supervision

Does Akka define what kind of things are an actor failures and what kind of strategies they recommend in general? What if the supervisor ignores the error? Is the decision about it explicit or implicit (omitting explicit handler)?

My entire use case for wanting this is not wanting to be have to have two separate handlers for errors, so just making sure your proposal covers it.

Could you give an example of "two separate handlers" being needed right now? I might miss that context.

[Baael]

Could you give an example of "two separate handlers" being needed right now? I might miss that context.

I think it is all about this:

invoke: {
    src: <Promise>,
    onDone: "done",
    onError: "error"
}

vs

invoke: {
    src: <Machine>,
    onDone: "done",
    on: {
        "error.child": {
            // handle what happens when an error occurs
            actions: (ctx, event) => {
                event.origin; // child ID
                event.data; // error data
            }
        }
    }
}

While working, refactoring, optimizing, reusing, it may be problematic.

[tivac]

@Andarist Sure!

What I want to be able to do

const child = Machine({
    initial : "request",

    states : {
        request : {
            invoke : {
                id : "req",
                src : () => {
                    /* something that explodes */
                },

                // Purposefully not handling the explosion because it's fatal,
                // the child cannot go on and the parent should be told so
            },
        },
    },
});

const parent = Machine({
    initial : "runchild",

    states : {
        runchild : {
            invoke : {
                id : "child",
                src : child,

                onError : "failure"
            },
        },
    },
});

How I have to do it right now:

const child = Machine({
    initial : "request",

    states : {
        request : {
            invoke : {
                id : "req",
                src : () => {
                    /* something that explodes */
                },

                onError : {
                    // This is awkward, because the "child" value has to match the id field
                    // defined in the parent machine for this invoked machine.
                    actions : (ctx, { data }) => sendParent(error("child", data)),
                },
            },
        },
    },
});

const parent = Machine({
    initial : "runchild",

    states : {
        runchild : {
            invoke : {
                id : "child",
                src : child,

                onError : "failure"
            },
        },
    },
});

What I have to do w/o the sendParent(error()) hackiness:

```jsconst child = Machine({
initial : "request",

states : {
    request : {
        invoke : {
            id : "req",
            src : () => {
                /* something that explodes */
            },

            onError : {
                actions : (ctx, { data }) => sendParent("ERROR", data),
            },
        },
    },
},

});

const parent = Machine({
initial : "runchild",

states : {
    runchild : {
        on : {
            // Required to handle runtime failures in the child
            ERROR : "failure",
        },

        invoke : {
            id : "child",
            src : child,

            // Required to handle JS failures in the child
            onError : "failure"
        },
    },
},

});
```

Now, these examples are all drastically simplified from what the real child/parent machines would look like and the amount of work that goes on in the error-handling paths, but hopefully it helps?

[davidkpiano]

I strongly believe that not being able to invoke a service because it failed to invoke (the "something that explodes") would be an SCXML "error.communication" (which would be in the internal event queue of the _parent_ that invoked the service):

Indicates that an error has occurred while trying to communicate with an external entity.

https://www.w3.org/TR/scxml/#ErrorEvents

XState should definitely raise that error. Thoughts @Andarist ?

[Andarist]

I strongly believe that not being able to invoke a service because it failed to invoke (the "something that explodes") would be an SCXML "error.communication" (which would be in the internal event queue of the parent that invoked the service):

This is definitely true when an external service can't be reached etc (e.g. using non-existent child ID as target). But if it has been reached and it "fails" during initialization (e.g. evaluating initial datamodel) then this failure is put (in form of error.execution) into the internal queue of the invoked service - because that error is already local to it.

If the value specified for a element (by 'src', children, or the environment) is not a legal data value, the SCXML Processor must raise place error.execution in the internal event queue and must create an empty data element in the data model with the specified id.
Note that <data> is a child of <datamodel> which in turn can be a child of <scxml>. I believe similar behavior can be found to be described around other executable contents (this is not limited to <data> evaluation).

The issue described here is not solely exclusive to "initialization" failures. From what I understand @tivac's intention - he'd like to "catch" any "error-events" in parent's external queue~ (or even catch such error automatically in grandparent).

What comes to my mind here - that maybe the problem itself is not about a need for manual conversion of Errors into specific error-events but rather about having to pass such events manually through layers of machines hierarchy, which I agree is tedious. This could be fixed by allowing to target arbitrary known machine as send's target (where at the moment you can only target own children and parent) - see https://github.com/davidkpiano/xstate/issues/523 . With that in place you could just forward explicit event to correct "supervisor".

[tivac]

having to pass such events manually through layers of machines hierarchy

This is definitely a big part of it, though I'm actually less annoyed by the need to explicitly pass errors up to the parent than I am by having two separate paths in the parent to handle different types of errors.

In my mind onError means "the child errored and I want to handle it", which is currently only true for some of the errors in the child unless you use the sendParent(error()) workaround.

[Andarist]

Could you specify when this gets currently called:

                // Required to handle JS failures in the child
                onError : "failure"

You name both types of errors 1) runtime failures and 2) JS failures - which ain't that easily distinguishable for a reader (like me).

[tivac]

Docs describe it like this:

onError - (optional) the transition to be taken when the invoked service encounters an execution error.

But... I don't know what an "execution error" is when invoking another machine as it turns out. I tried various forms of invalid things (invalid targets, invoking numbers as functions) and I'm not sure what can actually trigger an onError handler.

[Andarist]

I think that for invoked machines it just won't be called (right now) at all - so this leaves you with only one error handler in the parent 😉

The feedback received here is great ❤️ we'd really like to recognize the needs of the community to design the API with that in mind. This means that there probably won't be any "quick actions" taken here (maybe besides adjusting the docs), but be sure that we are going to improve the story (one way or another) around this.

[tivac]

https://codesandbox.io/s/xstate-sendparent-test-9mt6e

This is likely the shape of what I'll be using going forward to avoid the "child machine needs to know its id issue" with using the sendParent(error(<id>)) construct. It... works fine, but I think it's ugly & harder to read compared to being able to use onError like I wanted to. I especially dislike that if the parent invoke has an onDone transition defined that the "done" and "error" handlers are now in two different places in the tree instead of nicely encapsulated in the invoke.

I'm not sure why this bothers me so much, but it really does!

[davidkpiano]

Okay, borrowing some terminology from Akka, we can have an action creator called escalate() which escalates an error to the parent.

This would be the same as sendParent((_, event) => error(event)) and would be handled in:

// child
{
  onError: { actions: escalate() }, // error from event (default)
  // ...
  entry: escalate({ message: 'uh oh' }) // custom error
}

// ...

// parent
{
  src: someMachine,
  onError: {
    // handles the escalated error events
  }
}

Thoughts?

Besides the custom escalate() event, we should handle when the parent service is sent error(...) actions directly (via sendParent(...)).

[tivac]

Seems reasonable to me. I've got zero problems with it being an explicit thing for the child to fail and notify the parent. escalate seems like a better name for it then flameoutAndDie or something 👍🏻

[davidkpiano]

With the upcoming XState 4.7, your example will look like:

const child = Machine({
  initial: "die",
  states: {
    die: {
      entry: escalate("FATALITY")
    }
  }
});

const parent = Machine({
  initial: "one",

  states: {
    one: {
      invoke: {
        id: "child",
        src: child,
        // taken due to escalate(...)
        onError: "two"
      }
    },
    two: {}
  }
});

[tivac]

Thanks everybody, it took a bit to get alignment but I think this ended up in a great place!

[amitnovick]

A bit of a nitpick about the escalate(...) function arguments:

I find it intuitive that the argument would be the Error built-in JS object, with the signature:

escalate (error: Error): void

This is because semantically, we catch the escalation with an onError, so the user might think that they need to escalate an Error literally.

Of course, for sending a non-Error value to the parent service, one can always use sendParent.

[davidkpiano]

@amitnovick XState goes beyond the browser.

How would you communicate an error from a remote service, as plain JSON?

The point of escalate() is not the shape of the error (you can pass it an Error instance if you want), it's the invoke config automatically recognizing it in onError.

[amitnovick]

XState goes beyond the browser.

How would you communicate an error from a remote service, as plain JSON?

Ah, I didn't realize serializability was a goal in service communications, but now that you brought it up and I thought about it, I agree that it's useful in many cases. With serializable messages, there is no need to to use custom encoding and decoding and just rely on JSON.

[amitnovick]

@Andarist

I'm also not yet sure if supporting this is a good idea - errors can have different root causes, meanings etc (some are expected - e.g. network failure, some are unexpected - e.g. a typo) and explicit error handling (on per call site basis) should be IMHO preferred.

Here's a proposal which tries to preserve explicitness but enables onError to catch runtime exceptions/errors:

Communicated messages from the invoked service to the invoking parent can bear a special type value on the event object.

Currently we have the following type format denoting special message treatment: ${prefix}.${id}, with at least these two prefixes (there may be more I'm unaware of):

• ActionTypes.DoneInvoke gets handled by onDone
• ActionTypes.error gets handled by onError

In order for the parent to handle a runtime exception/error thrown from an invoked service, it could do something like this:

import { isRuntimeError } from 'xstate'

// ...

invoke: {
  src: machineThatMightThrowAnError,
  onError: (_, event) => {
    if (isRuntimeError(event.type)) {
      //handle the error
    }
  }
}

Where

isRuntimeError(type: string): boolean

would be positive when type has a specific prefix, e.g. ActionTypes.error.runtime.

[davidkpiano]

@amitnovick Keep in mind that onError is a _transition(s)_ configuration, not a function.

[amitnovick]

D'oh! 🤦

It could still work using Guards though:

invoke: {
  src: machineThatMightThrowAnError,
  onError: {
    cond: (_, event) => isRuntimeError(event.type),
    target: 'somethingWrongHappenedScreen'
  }
}

Note: implementing this would be a breaking change, since onError would be called in cases where it currently doesn't.

[vivek12345]

Although this issue is closed with the new release which now supports escalate.
I wanted to understand how can I pass the exact error event to the onError of parent.
I want to do something like this

failure: {
      entry: {
           actions: (ctx, event) => {
               escalate(event);
           }
      }
}

or like this

onError: {
     actions: (ctx, event) => {
          escalate(event);
      },
      target: "failure" 
}

The reason is I want the parent to get the exact error object that got returned when let's say an api call failed.
Can we do this?

[davidkpiano]

@vivek12345 Can you open a separate issue for that enhancement request?

Also please keep in mind that escalate(...) and all other action creators are action creators. They do not actually execute the action.

[vivek12345]

Sure I will create a new request for this. Thank you