fix(core): errored initial snapshot throws sync w/o observers #5045

boneskull · 2024-08-17T23:09:09Z

This modifies the error-handling logic for when an error is thrown during the creation of an Actor's initial snapshot (during start()). If the actor has no observers, the error will now be thrown synchronously out of start() instead of to the global error handler.

Example use case:

const machine = createMachine({
  context: () => {
    throw new Error('egad!');
  }
});

const actor = createActor(machine);

try {
  await toPromise(actor.start());
} catch (err) {
  err.message === 'egad!' // true
}

Note that this does not impact child actors, but it might want to!

Fixes: #4928

changeset-bot · 2024-08-17T23:09:13Z

⚠️ No Changeset found

Latest commit: 0dabaa7

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

boneskull · 2024-08-17T23:10:55Z

For reference, the current workaround is this:

const machine = createMachine({
  context: () => {
    throw new Error('egad!');
  }
});
const actor = createActor(machine);

const p = toPromise(actor);
actor.start();
try {
  await p;
} catch (err) {
  err.message === 'egad!' // true
}

boneskull · 2024-08-17T23:11:13Z

I don't know if the new behavior is desirable to the maintainers, but I find it more ergonomic. Thought I would restart that conversation here!

This modifies the error-handling logic for when an error is thrown during the creation of an Actor's initial snapshot (during `start()`). _If_ the actor has _no_ observers, the error will now be thrown synchronously out of `start()` instead of to the global error handler. Example use case: ```js const machine = createMachine({ context: () => { throw new Error('egad!'); } }); const actor = createActor(machine); try { await toPromise(actor.start()); } catch (err) { err.message === 'egad!' // true } ``` Note that this _does not impact child actors_. Fixes: statelyai#4928

davidkpiano

I agree with this PR in principle; waiting to see what @Andarist thinks

boneskull · 2024-08-18T20:07:45Z

My main concern is that it is somewhat inconsistent, since if the example machine was spawned as a child actor, it would reach the global error handler.

...that being said, if using spawnChild(), there's really nowhere else for the error to go.

boneskull · 2024-08-19T04:51:03Z

This looks like a breaking change to me unless you feel this is actually a "bug fix" and not just a "revision of intent" 😜

Andarist · 2024-08-19T09:30:47Z

...that being said, if using spawnChild(), there's really nowhere else for the error to go.

Could you show the exact situation you are talking about here?

boneskull · 2024-08-19T20:27:30Z

...that being said, if using spawnChild(), there's really nowhere else for the error to go.

Could you show the exact situation you are talking about here?

@Andarist

something like this:

it('handled sync errors thrown when starting a child actor should be reported globally when spawned via spawnChild()', (done) => {
  const badMachine = createMachine({
    context: () => {
      throw new Error('handled_sync_error_in_actor_start');
    }
  });
  
  const machine = createMachine({
    entry: [
      spawnChild(badMachine)
    ]
  });

  const actorRef = createActor(machine);
  actorRef.start();
  installGlobalOnErrorHandler((ev) => {
    ev.preventDefault();
    expect(ev.error.message).toEqual('handled_sync_error_in_actor_start');
    done();
  });
});

If a xstate.error.actor.badMachine (or w/e) event handler in machine is the canonical way to trap this error, then I don't think changes to the above behavior are warranted (i.e. changing it so the error is thrown out of actor.start(), which I think is only possible because it's thrown synchronously?). It's just that there's no other reasonable way to catch it out of the root actor (e.g. if badMachine was the root actor), which is what this PR solves.

davidkpiano · 2024-08-23T14:22:19Z

packages/core/src/createActor.ts

+        // **if the actor has no observer when start() is called**, this error would otherwise be
+        // thrown to the global error handler. to prevent this--and allow start() to be wrapped in try/catch--we
+        // create a temporary observer that receives the error (via _reportError()) and rethrows it from this call stack.


I'm wondering if we can do this a different way without creating a temp sub

@davidkpiano The folded section in this comment has an alternate proposal, which I explored but didn't get working fully.

Andarist · 2024-08-29T22:29:43Z

packages/core/test/errors.test.ts

+  it('error thrown when resolving the initial context should reject when wrapped in a Promise', async () => {
+    const machine = createMachine({
+      context: () => {
+        throw new Error('oh no');
+      }
+    });
+
+    const actor = createActor(machine);
+
+    try {
+      await toPromise(actor.start());
+      fail();
+    } catch (err) {
+      expect((err as Error).message).toEqual('oh no');
+    }
+  });


This doesn't work like you think it does. This error is still very much thrown synchronously. toPromise isn't ever called during this test case.

I'm not sure what you mean. If createActor threw synchronously, wouldn't this test fail?

IIRC, It's the actor.start that throws, before its return value can even get passed to toPromise

Andarist · 2024-08-29T22:31:57Z

packages/core/test/errors.test.ts

+  it('error thrown when resolving the initial context should rethrow synchronously', (done) => {
+    const machine = createMachine({
+      context: () => {
+        throw new Error('oh no');
+      }
+    });
+
+    const actor = createActor(machine);
+
+    installGlobalOnErrorHandler(() => {
+      done.fail();
+    });
+
+    expect(() => actor.start()).toThrowErrorMatchingInlineSnapshot(`"oh no"`);


actors are always async conceptually, a synchronous throw is something that might sound convenient at first but you essentially create 2 error channels by introducing this. Now, if you want to handle errors, u need to both install an error listener and wrap this in try/catch. The guiding principle that we had when designing this was that you should always be notified about errors in the very same way (through error listeners).

@Andarist In theory I agree, but in practice, if we know that an actor is going to fail right at the start, rather than later during event processing, it's a much better DX (IMO) to throw right away.

This is my feeling too. User experience > theoretical purity

@boneskull I may have changed my stance on this: https://www.youtube.com/watch?v=JCXZhe6KsxQ&t=294s

I don't follow

@davidkpiano Should I close this?

Yes, let's close it for now, given that it's a breaking change.

boneskull force-pushed the boneskull/issue-4928 branch from 07acff0 to 0dabaa7 Compare August 17, 2024 23:16

davidkpiano reviewed Aug 18, 2024

View reviewed changes

davidkpiano reviewed Aug 23, 2024

View reviewed changes

Andarist reviewed Aug 29, 2024

View reviewed changes

davidkpiano closed this Oct 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(core): errored initial snapshot throws sync w/o observers #5045

fix(core): errored initial snapshot throws sync w/o observers #5045

boneskull commented Aug 17, 2024 •

edited

Loading

changeset-bot bot commented Aug 17, 2024 •

edited

Loading

boneskull commented Aug 17, 2024 •

edited

Loading

boneskull commented Aug 17, 2024

davidkpiano left a comment

boneskull commented Aug 18, 2024 •

edited

Loading

boneskull commented Aug 19, 2024

Andarist commented Aug 19, 2024

boneskull commented Aug 19, 2024 •

edited

Loading

davidkpiano Aug 23, 2024

boneskull Aug 29, 2024

Andarist Aug 29, 2024

boneskull Sep 11, 2024

Andarist Sep 12, 2024

Andarist Aug 29, 2024

davidkpiano Aug 30, 2024

boneskull Sep 11, 2024

davidkpiano Sep 11, 2024

boneskull Sep 13, 2024

boneskull Oct 7, 2024

davidkpiano Oct 7, 2024

fix(core): errored initial snapshot throws sync w/o observers #5045

fix(core): errored initial snapshot throws sync w/o observers #5045

Conversation

boneskull commented Aug 17, 2024 • edited Loading

changeset-bot bot commented Aug 17, 2024 • edited Loading

⚠️ No Changeset found

boneskull commented Aug 17, 2024 • edited Loading

boneskull commented Aug 17, 2024

davidkpiano left a comment

Choose a reason for hiding this comment

boneskull commented Aug 18, 2024 • edited Loading

boneskull commented Aug 19, 2024

Andarist commented Aug 19, 2024

boneskull commented Aug 19, 2024 • edited Loading

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

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

boneskull commented Aug 17, 2024 •

edited

Loading

changeset-bot bot commented Aug 17, 2024 •

edited

Loading

boneskull commented Aug 17, 2024 •

edited

Loading

boneskull commented Aug 18, 2024 •

edited

Loading

boneskull commented Aug 19, 2024 •

edited

Loading