assign failed silently during async transition

[connectdotz]

Description

when invoking an async function (returns a promise), upon onDone(), it tried to update the context with the promise result. If for some reason the code failed (unintended, such as a coding error), the error is not be captured nor reported (at least during testing, possibly in a production run as well)

see https://codesandbox.io/s/silent-failure-example2-g0p9d test run, the async operation should report error test case failed not by exception but by timing out (xstate is not advancing the state nor reporting error)

Expected Result

either invoke onError() of the invoke or terminate the interpreter as the sync-operation does.

Actual Result

the interpreter does not advance state nor reporting error.

Reproduction

https://codesandbox.io/s/silent-failure-example2-g0p9d

Additional context

original gitter discussion: https://gitter.im/statecharts/statecharts?at=5f6a14c64002c640b5dd60b3

[Andarist]

onError is a synctatic sugar for a transition for receiving an error event from the invoked service - this error happens in the parent machine though and this it shouldnt be triggered in my opinion, even though this might sound counter intuitive at first.

As this error has not been handled properly in the machine that has causes it (the parent one here) it should probably fail that machine, stop it, etc. Its part of the bogger story for v5 - we need to design error handling properly, so the rules can be written down and known to people. Changing this right now - in v4 - would be a breaking change.

[danielkcz]

I would slightly clarify here. It's not related to only invoking an async function, but effectively anything that turns the machine into an asynchronous service. Notice that by adding entry: 'update-counts' in codesandbox above to the root state node, the test will pass because the error was thrown synchronously.

As this error has not been handled properly in the machine that has causes it (the parent one here) it should probably fail that machine, stop it, etc.

I think that makes sense, but still, you need some async-aware outlet. Anything thrown while service runs asynchronously will be swallowed unless caught on that promise chain directly.

[danielkcz]

Anything thrown while service runs asynchronously will be swallowed unless caught on that promise chain directly.

Or to be precise, it might show up in the console in some cases as unhandledRejection, but AFAIK future NodeJS versions want to drop that outlet.

The problem is if you run a synchronous test that cannot see the internal promise chain and as such, it cannot wait for it and catch error on it.

I think that ultimately the service.send (maybe some others) needs to return a Promise so it can be awaited and used for catching errors.

In the React world, they solved this nicely with act utility that effectively hijacks all async crap and allows them to wait for it with nice API, but that's probably overkill for XState needs.

[Andarist]

Hm, I'm not sure if I understand your concerns @FredyC , could you clarify them more? From what I understand @connectdotz issue is not really about being able to make test assertions on anything, the test is just written to showcase a particular behavior and the issue itself is about that behavior. Quoting the "expected outcome" by the OP:

either invoke onError() of the invoke or terminate the interpreter as the sync-operation does.

[danielkcz]

Well, the OP is a valid use case, but I am pondering on a wider concern here.

You see, as long as the machine is synchronous (simple transitions between different states), the service itself stays synchronous, and any error thrown within action propagates synchronously. That means it can effectively stop the machine and die.

But at the moment you introduce asynchronicity, the actions that happen to throw can only pass that error to the Promise chain. It can no longer throw it synchronously.

These are call stacks and you can clearly see the difference where Promise turns the whole thing into an asynchronous process.

---

My general concern here is not about handling these errors but to be able to identify them without them getting lost. It's painful to not know why action failed and you don't know why. Technically it might be enough if the error is just output with console.error.

[Andarist]

Could u prepare a repro case of what you are experiencing? I think I might still be missing something. Rejected promises should trigger onError handlers correctly so those wouldnt be lost.

[danielkcz]

The codesandbox in OP is already showing this problem. That's where I got that latter stack trace. Ignore onError concern for now, because that's not supposed to catch errors within actions anyway, correct? If you switch to the test tab in CS, the test fails, because it was unable to catch the error, synchronously.

[connectdotz]

If you switch to the test tab in CS, the test fails, because it was unable to catch the error, synchronously.

I see what might be the confusion... The try/catch will never catch the async errors, unless like you said if the send() method returns a promise, which I think makes sense too, or maybe a separate API like sendAsync().

Ignore onError concern for now, because that's not supposed to catch errors within actions anyway, correct?

I guess it depends, if I translate the invoke into the following structure, the onError, i.e. catch should catch the error from both the promise and the onDone...

somePromise()
  .then(() => {
    //onDone, where the action is created ? executed?
  }).catch(e => {
    //onError
  });

But of course, if onDone only created the faulty action instead of executing it right there, the onError will not be able to catch the error... In that situation, and maybe many other async use cases, I wonder perhaps the simplest solution is to just listen to the unhandledRejection (the warning behavior is going to be deprecated but not the event itself) or uncaughtException event, like you mentioned, at least to output with console.error. Maybe we can also consider adding a system-termination state or an onError on the StateNode for the uncaught error, much like the onDone to report reaching the final state... just some thoughts

[Andarist]

I see what might be the confusion... The try/catch will never catch the async errors, unless like you said if the send() method returns a promise, which I think makes sense too, or maybe a separate API like sendAsync().

This doesn't make a conceptual sense though. send can't return a promise because it doesn't await for anything - there is no moment in which we could consider it to be resolved. Basically think of send always as an async operation - it's only synchronous because we don't force it to be async, but conceptually it totally is. The mere meaning of it is "schedule an event on this target", but that target can be anywhere - it doesn't even have to live on your machine.

Maybe we can also consider adding a system-termination state or an onError on the StateNode for the uncaught error, much like the onDone to report reaching the final state... just some thoughts

Some sort of such behavior would be most desired I believe.

---

If you switch to the test tab in CS, the test fails, because it was unable to catch the error, synchronously.

So actually... no error should be even possible to be caught synchronously by a catch block wrapping the service.send. As mentioned here - send is always considered to be async. If we consider SCXML spec - all runtime errors should be converted to error.execution events and should be just raised on the machine. I don't quite believe that it's perfect though because it means that if you don't handle this special event then errors are effectively just swallowed. That's I believe that the error handling should be reconsidered by us and we should stop the machine and propagate error to the consumer through other channels (first try to propagate to a parent etc, so it could handle a failure in its onError handler). This is just tentative thought on my side though (the last part). The whole topic needs deep analysis.

[danielkcz]

I don't quite believe that it's perfect though because it means that if you don't handle this special event then errors are effectively just swallowed. That's I believe that the error handling should be reconsidered by us and we should stop the machine and propagate error to the consumer through other channels

I am glad we are finally on the same page 😅

For V4 it might make sense a quick patch where error caught in actions is at least logged with console.error so we are not driving in blind in case a mistake happens. In DEV mode only should be good enough.

[Andarist]

For V4 it might make sense a quick patch where error caught in actions is at least logged with console.error so we are not driving in blind in case a mistake happens. In DEV mode only should be good enough.

But arent those logged automatically? Those are just uncaught errors right now so should be logged by the browser. I've adjusted the @connectdotz codesandbox' to switch to the sync mode and that error is being logged:
https://codesandbox.io/s/silent-failure-example2-forked-hcnlp?file=/src/index.ts:402-423

[danielkcz]

Yes, but async actions are a problem since the beginning of this issue and those are not logged anywhere, just silently swallowed. That's why am I suggesting adding console.error for V4.

Oh wait, you have to open devtools console in CS to see that. I guess it would be seen in the normal browser as well. But it's wildly inconsistent. Sync error is in CS console, async in devtools only. It's easy to miss that.

[Andarist]

That has to be some CS quirk though, the error is logged correctly to the browser's console in both cases and if we'd add an additional console.error then you'd end up with duplicates.

[danielkcz]

@connectdotz I wonder what was your initial motivation for this? I mean you said it's not about being able to catch that error in the test but to know about an unexpected error that happened in the action. Didn't you get that error spilled in the console?

[connectdotz]

It's a good question why the browser shows exception but the Test doesn't? One can find some clue from the bottom of the error panel in the browser tab:

This screen is visible only in development. It will not appear if the app crashes in production.
Open your browser’s developer console to further inspect this error.
This error overlay is powered by `react-error-overlay` used in `create-react-app`.

I took a quick look at react-error-overlay, and the magic seems to be where we suspected: it registered to window's uncaught error, such as the unhandledRejection (here).

Jest did not do that so we won't know there is an uncaught exception for async operations during testing.

Sure we can defer the responsibility to the other runtime containers or ask our users to deal with it themselves (with enough console.log, one can find the problem eventually)... But I think if xstate can detect and help users to quickly realize and pinpoint the problem, without relying on the underlying engine, it will save everybody a lot of time thus make xstate a much better framework...

regarding double reporting, if these errors are reported to the optional onError, which can override the default console.error, then users can easily change how they are reported if it became a problem.

[danielkcz]

Silently logging the error would be a bad idea IMO.

Never heard the term "silent logging" :) I suppose you mean that it doesn't stop anything and code around is moving forward. However, errors in actions are already behaving like that since you cannot catch them. In some environments (why this issue exists) it's even more painful to rely on "unhandledRejection" to appear somewhere.

I'd still like to see what a more ideal API for this would be. Seems like .onError(...) covers this

I have no problem with that, that will certainly help in many cases. My concern is that you need to explicitly set it up for every machine. Not many people will realize they need to do that. Unless of course, you plan to add an implicit handler to useMachine React binding which could be technically set up to convert async error to sync one.

[davidkpiano]

...which could be technically set up to convert async error to sync one.

This is what I'm confused about. You cannot treat async errors as sync (i.e., via try/catch), unless you explicitly have a callback that handles them.

So let's resolve on adding an .onError(...) handler. We can probably discuss further DX after that, but I don't think that XState should be responsible for doing anything magical for async errors, besides stopping the running service, of course.

Unless you can demonstrate what converting an async error to a sync one would look like?

should xstate at least report (console.error) it? I think we probably also agree on this point. At least a default behavior if nothing else.

Sure, this can be done.

Since this is the uncaught/unexpected error, shouldn't it render the machine's/interpreter's own state indeterministic? As a state machine engine, one would think xstate component should transition itself to something like an 'internal-error' state that is final and maybe with an entry point to print out the error...? No need to rethrow, as the error is being "handled" by advancing the machine state accordingly.

We shouldn't define custom states for this. The interpreter should stop. But the error should still be "visible" - we shouldn't silence the error.

The relevant part of the SCXML spec is here: https://www.w3.org/TR/scxml/#ErrorEvents

Note however that authors can arrange for otherwise unhandled errors to cause the interpreter to exit by creating a transition with "event" attribute of 'error' and a target of any top-level final state (i.e. one that is a child of ). If such a transition T is placed in a state S, it will cause the state machine to terminate on any error that is raised in S or one of its substates and is not handled by another transition that is placed in a substate of S or in S and preceding T in document order.

---

So here is my proposal:

1. Add service.onError(errorHandler) for capturing unhandled errors.
2. Allow onError: ... properties on any state node to capture errors.
3. If none of the above are present, unhandled errors should be thrown (as is currently done), but with extra information about the machine/state/action/etc. where the error occurred.

[connectdotz]

@davidkpiano a few questions:

1. In your proposal, what would happen if neither 1 nor 2 occurred? for sync operations, the rethrow will terminate the interpreter like it is now; but what will happen for the async operation? the rethrow will not terminate the interpreter, right? Will users see some logging or it will just fail "silently" (from a user's perspective) like it is now?

2. Seems like you prefer introducing a new callback API than a new state? If I read the SCXML spec (thanks for the link, it is quite helpful) correctly, it seems to recommend that the uncaught error could trigger a state transition to a "top-level final state", which caused the interpreter to "exit" (as it reached the final state). It did not mention a callback approach... Do you feel we need to use callback because of the async operations? Will the state approach not working for async operations?

[davidkpiano]

but what will happen for the async operation? the rethrow will not terminate the interpreter, right?

An async error is an event like any other. If it is not handled (i.e., in onError: ...) then nothing happens.

Will users see some logging or it will just fail "silently" (from a user's perspective) like it is now?

Yeah, that's reasonable. We should add logging.

Do you feel we need to use callback because of the async operations?

The .onError(cb) would basically be for convenience - capturing any error that occurs and avoiding uncaught async errors (which was part of the original concern). But it can certainly be an alternative to adding a specific error state:

createMachine({
  // ...
  onError: {
    actions: (_, e) => console.error(e),
    target: '.someFinalState'
  }
});

You're right; as is described in SCXML, adding an explicit "error handler" transition should be preferred.

[danielkcz]

It would certainly be nice if we could explicitly transition to an error state no matter where the error happened in the machine. I suppose the onError as callback would allow using send too, but that's less convenient.

Also ... what happens if error handler has error? :) Should be covered somehow so we don't end up in the loop.

[davidkpiano]

It would certainly be nice if we could explicitly transition to an error state no matter where the error happened in the machine.

Yes, that would work with the top-level onError transition.

Also ... what happens if error handler has error? :) Should be covered somehow so we don't end up in the loop.

Sigh, one thing at a time 😅 I'll consider that too.

[danielkcz]

And what about currently available onError for invocations? Will this top-level handler "win", or both will run?

I would say that if a specific handler is declared, it should be only one to run. Otherwise, it would fallback to top-level one.

[davidkpiano]

The transition resolution follows the exact same semantics as SCXML. It goes from leaf-node to root-node; most-specific to least-specific.

If an error event is handled in a state, it will only be handled by that transition.

Otherwise, it propagates to the nearest ancestor that handles the error.

[connectdotz]

@davidkpiano I think the idea of making onError available to handle the user-state (expected) error is good, but see a few potential problems to use it for uncaught-error:

• if users did not add onError in their machines, does the uncaught error, again, failed silently?
• if xstate then implement a "default" onError transition as the answer to the above question, what is the target state it is supposed to transition to? Some kind of system-defined final state? The onError doesn't really spare the system to create a new state then.
• for users to see the uncaught error, they will have to add onError and a final state, most likely different from their normal final state... So the onError doesn't really spare users to create a new state.
• what if users define an onError that did not transition to a final state (common if they use onError to capture the expected error like in the invoke-error-retry patten)? Now they have to check what kind of error it is and forced to handle the uncaught error in every onError? Or worse, it fails to distinguish the errors and transitioned to the wrong state...

Therefore, while I don't object onError as a general error handling mechanism for user-states, I do wondering if to tunnel all uncaught error via this transition is the best, simplest, or the least intrusive solution?

[danielkcz]

@connectdotz I think it's generally correct to have at least one error state in each machine (except simple ones) used whenever something goes wrong. You can even have "retry" logic implemented with that.

Also if I understand correctly from the above snippet, it won't be mandatory to specify target. You could just define the action that can be used for logging or even sending it to a reporting service of yours.

[davidkpiano]

Right; let's not complicate this. The scope is getting too large, so for v4, let's limit it to this:

• An onError transition can be added to any state node. If you want to capture any error, add it to the root node:

const machine = createMachine({
  // ...
  states: {/* ... */},
  onError: {
    // top-level error catcher
  }
});

• If you don't add onError, the same behavior as today will happen. We can discuss if this behavior should change in a separate issue after the above is released, but I want to keep the scope down for this one, as there already is too much churn/discussion.

Again, XState is about being explicit, so if you don't explicitly add a transition to capture error events, then the normal (arguably undesirable) behavior should happen.

[connectdotz]

Also if I understand correctly from the above snippet, it won't be mandatory to specify target.

Without the target the machine will not transition to a final state (exit) and thus remain in the wrong state, isn't it? Isn't this directly violate SCXML recommendation that it should lead to exit/final state (to maintain the state machine integrity)?

Right; let's not complicate this. The scope is getting too large...

indeed, introducing onError for every StateNode is not trivial, as there are many implications...

Again, XState is about being explicit,

It seems this is the main reason that @davidkpiano feels strongly against a new system-state approach... I can see your point but let me make one last argument 😅 ... it might be actually desirable to make an exception for the uncaught-error state, which is mostly a development-stage scenario, so the machine definition won't need to be polluted...

ok, that's all I am going to say about this issue 🤐 . Whatever you guys decided to do that is better than what we have today is great. I appreciate your patient to discuss and continue to improve this product. 🙏

[davidkpiano]

Without the target the machine will not transition to a final state (exit) and thus remain in the wrong state, isn't it? Isn't this directly violate SCXML recommendation that it should lead to exit/final state (to maintain the state machine integrity)?

It's a recommendation; it's up to the developer to specify a final state target if they feel it's appropriate.

It seems this is the main reason that @davidkpiano feels strongly against a new system-state approach... I can see your point but let me make one last argument 😅 ... it might be actually desirable to make an exception for the uncaught-error state, which is mostly a development-stage scenario, so the machine definition won't need to be polluted...

No exceptions. In the case that an error is not caught by any onError, the error will be thrown and the service will simply stop. In a way, you can consider that the "uncaught-error state".

ok, that's all I am going to say about this issue 🤐 . Whatever you guys decided to do that is better than what we have today is great. I appreciate your patient to discuss and continue to improve this product. 🙏

Thanks! I'll make a PR and we can bikeshed later, especially for v5.

[Andarist]

I must say that I still don't quite understand the whole discussion about adding logging for uncaught errors. From what I've checked this is exactly how it behaves right now - because it's the default behavior of most environments. I'm not aware of any modern environment that swallows UnhandledPromiseRejectionWarning entirely. And a lot of discussions here have revolved around this while I believe this just shouldn't be an issue. Improved logging with more context - sure, that could be done and discussed separately. But as to the sole fact if errors are "swallowed" or not currently - to the best of my knowledge: no, they are not. They should always be surfaced to the developer in the console.

---

It's a recommendation; it's up to the developer to specify a final state target if they feel it's appropriate.

Right, it's a recommendation in the SCXML but - as @connectdotz has noticed - this might not be enough for our default. I feel that recommendation favors unsafe behavior over safer ones. If any error is left without being handled by a specific user handler then the model is broken - repercussions are unknown and they might be severe for the application, user flow, or whatever. It's by far too easy to forget adding a proper error handling and thus leave your machines in the inconsistent states if we don't stop such broken machines from executing anything further.

But also you have expressed similar desire here:

We shouldn't define custom states for this. The interpreter should stop. But the error should still be "visible" - we shouldn't silence the error.

The whole conversation became lengthy and quite hard to track for me so I'm not even sure what are exact, final, opinions of all people involved here 😅

[davidkpiano]

Yeah, let's move this to discussions instead - there is nothing that is clearly actionable here. (EDIT: moved!)