statelyai / xstate

Actor-based state management & orchestration for complex app logic.
https://stately.ai/docs
MIT License
26.51k stars 1.22k forks source link

Bug: actors leak uncaught errors #4928

Open boneskull opened 4 weeks ago

boneskull commented 4 weeks ago

XState version

XState version 5

Description

Given:

  1. Root machine actor A
  2. No error subscriber is registered with A
  3. A's initial context assignment function throws an error E

E cannot be caught by any mechanism other than a global error listener (e.g., window.onerror or process.on('uncaughtException').

Even if a machine actor is wrapped in toPromise(), E is still thrown this way in addition to being rejected out of the Promise.

This behavior does not occur if the machine actor has a subscriber with an error listener.

I really don't want to create a bunch of subscriptions where toPromise() would work for my purposes. 😄

Expected result

The error thrown by the context initializer should not be thrown in addition to being rejected.

Actual result

The "extra" error thrown is only catchable via a global error handler.

Reproduction

https://stackblitz.com/edit/github-trfney?file=src%2Fmain.ts

Additional context

This may be expected behavior. I understand that an actor that is started without an error listener and isn't wrapped in a Promise has no other choice but to throw an error this way; there's nowhere for it to go.

In Node.js, an EE will emit an error event, which can be listened to. If it is listened to, then the EE won't throw an uncaught exception. However, the once wrapper (which is only vaguely analogous to toPromise()) will also avoid the uncaught exception:

import {once, EventEmitter} from 'node:events';

const emitter = new EventEmitter();

// without this, the error would be uncaught
once(emitter, 'error').then(err => {
  // handle error
});

emitter.emit('error', new Error('handled'));

That's why I think this is a bug. Like above, toPromise should act like a subscriber with an error listener.

It may also not be unique to the context assignment function, either...

boneskull commented 4 weeks ago

I updated the title to better describe what I think is happening; my hunch is that it can be fixed in the toPromise implementation.

boneskull commented 4 weeks ago

It may be worth mentioning why the context assignment function would ever want to throw. Because:

const machine = setup({
  types: {
    input: {} as {signal: AbortSignal}
  }
}).createMachine({
  context: ({input: {signal}}) => {
    if (signal.aborted) {
      throw new AbortError(signal.reason);
    }
  }
});
Andarist commented 4 weeks ago

I updated the title to better describe what I think is happening; my hunch is that it can be fixed in the toPromise implementation.

It can't be fixed there. This error is thrown synchronously. By the time toPromise gets actually called in this example... it's already too late.

You can (likely) work around this problem like this:

const actor = createActor(TestMachine);
const p = toPromise(actor);
actor.start();
await p;
boneskull commented 4 weeks ago

@Andarist Not sure I love it, but I can confirm that workaround works: https://stackblitz.com/edit/github-trfney-kh9qur?file=src%2Fmain.ts

Might want to update the docs and/or add a warning about this if you don't plan to address it otherwise 😄

Andarist commented 4 weeks ago

To be clear, I don't necessarily say I love it either 😉