Bug: actors leak uncaught errors

[boneskull]

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]

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]

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]

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]

@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]

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

[boneskull]

@Andarist

It's not thrown synchronously, though:

https://github.com/statelyai/xstate/blob/3e5424d7a4bd6dde63c2ada6b6a85f55ae42f7ee/packages/core/src/reportUnhandledError.ts#L9-L13

UPDATE: nevermind; this doesn't work

Looking at this further, I think it could be solved by modifying Actor._error() and Actor._reportError() so that they accept a flag parameter. _error() passes the flag through to _reportError(). _reportError() then re-throws the error if the flag is true:

  private _error(err: unknown, sync = false): void {
    this._stopProcedure();
    this._reportError(err, sync);
    // ...
  }
  
  private _reportError(err: unknown, sync = false): void {
    if (!this.observers.size) {
      if (!this._parent) {
        if (sync) {
          throw err;
        }
        reportUnhandledError(err);
      }
      return;
    }
    // ...
  }  

Back in Actor.start():

https://github.com/statelyai/xstate/blob/3e5424d7a4bd6dde63c2ada6b6a85f55ae42f7ee/packages/core/src/createActor.ts#L493-L495

The error case can be changed to this:

      case 'error':
        this._error((this._snapshot as any).error, true);
        return this;

It's not an elegant API by any means, but will only throw synchronously if the actor is already in an error state at startup and there are no active subscriptions.

[boneskull]

Another way to solve it may be to create a transient, internal error subscription at the beginning of Actor.start() (if no other subscribers exist) which synchronously re-throws. The subscription would unsubscribe when start() exits.

[boneskull]

The second suggestion works without gumming up existing error handling:

      case 'error':
        let err: unknown | undefined;
        let tempSub: Subscription | undefined;
        if (!this.observers.size) {
          tempSub = this.subscribe({error: error => {
            // avoid the thing that catches errors thrown from error observers
            err = error;
          }});
        }
        this._error((this._snapshot as any).error);
        tempSub?.unsubscribe();
        if (err) {
          throw err;
        }
        return this;
    }

and:

  it('throws errors synchronously if no observers are present', () => {
    const machine = createMachine({
      context: () => { 
        throw new Error('oh no'); 
      }
    });

    const actor = createActor(machine);
    expect(() => actor.start()).toThrowErrorMatchingInlineSnapshot(`"oh no"`);
  })

[matheo]

@Andarist I'm having a leaked error with a spawned actor (I've tried fromObservable or fromCallback assigned childs). I'm trying to make a http request and if it throws, my main state-machine exits with an undefined event :cry:

myState: {
    entry: [
        assign({
            registerAssign: ({ spawn }) => spawn('registerAssignActor', { id: 'register-assign' }),
        }),
    ],
    exit: [
        stopChild('register-assign'),
        assign({ registerAssign: registerAssignRef }),
    ],

I'm able to catch the error event with the wildcard on: '*' but the event is undefined on the parent machine.

[davidkpiano]

@Andarist I'm having a leaked error with a spawned actor, no matter if I use fromObservable or fromCallback, I'm trying to make a http request and if it throws, my main state-machine exits with an undefined event 😢

Are you adding an error handler?

actor.subscribe({
  error: (err) => {
    // handle error
  }
});

actor.start();

[matheo]

@davidkpiano I'm spawning the actor by reference and assigning it to the context.

[Andarist]

@matheo please share a runnable repro case of the problem. It's hard to tell what you are observing based on a code slice.

[matheo]

@Andarist thanks! while building the repro I've realized an error displaying the child actor state, so the undefined event was not a big deal because it doesn't trigger anything and maybe it was related to the child actor being stopped/cleaned up.