Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed an issue with actors not being reinstantiated correctly in a microstep #3374

Merged
merged 1 commit into from
Jun 1, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/twelve-dingos-fold.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'xstate': patch
---

Fixed an issue with actors not being reinstantiated correctly when an actor with the same ID was first stopped and then invoked/spawned again in the same microstep.
5 changes: 4 additions & 1 deletion packages/core/src/StateMachine.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@ import {
resolveMicroTransition,
resolveStateValue,
toState,
transitionNode
transitionNode,
setChildren
} from './stateUtils';
import type {
AreAllImplementationsAssumedToBeProvided,
Expand Down Expand Up @@ -305,6 +306,8 @@ export class StateMachine<
preInitial._initial = true;
preInitial.actions.unshift(...actions);

setChildren(preInitial.children, actions);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The preInitial wasn't previously populated with children and thus without this change we were failing the adjusted logic here:
https://github.com/statelyai/xstate/pull/3374/files#diff-fc91146eff717b2bcd6adbc0955ca0549346ad7bef9aa2278e0a75a9793a66b9R656


return preInitial;
}

Expand Down
5 changes: 1 addition & 4 deletions packages/core/src/actions/invoke.ts
Original file line number Diff line number Diff line change
Expand Up @@ -62,10 +62,7 @@ export function invoke<
type,
params: {
...params,
id: params.id,
src: params.src,
ref: new ObservableActorRef(behavior, id),
meta
davidkpiano marked this conversation as resolved.
Show resolved Hide resolved
ref: new ObservableActorRef(behavior, id)
}
} as InvokeActionObject;
}
Expand Down
8 changes: 4 additions & 4 deletions packages/core/src/actions/stop.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,14 +32,14 @@ export function stop<
{
actor
},
({ params, type }, context, _event) => {
const actorRefOrString = isFunction(params.actor)
({ params, type }, context, _event, { state }) => {
const actorRef = isFunction(params.actor)
? params.actor(context, _event.data)
: params.actor;
: state.children[params.actor];
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we have to resolve a string to the ref here, before we mutate the new children in setChildren that was added here:
https://github.com/statelyai/xstate/pull/3374/files#diff-6f3a03f5c8d5024e53467d74c187685ab0acd0e1efb2549fbf2842d9616ad8dbR1635

Because that setChildren removes stopped children from what is going to become service.state.children we are no longer able to resolve it in the "stop handler" here:
https://github.com/statelyai/xstate/pull/3374/files#diff-fc91146eff717b2bcd6adbc0955ca0549346ad7bef9aa2278e0a75a9793a66b9L683


return {
type,
params: { actor: actorRefOrString }
params: { actor: actorRef }
} as StopActionObject;
}
);
Expand Down
36 changes: 8 additions & 28 deletions packages/core/src/interpreter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -632,7 +632,7 @@ export class Interpreter<
[actionTypes.cancel]: (_ctx, _e, { action }) => {
this.cancel((action as CancelActionObject).params.sendId);
},
[actionTypes.invoke]: (_ctx, _e, { action, state }) => {
[actionTypes.invoke]: (_ctx, _e, { action }) => {
const {
id,
autoForward,
Expand All @@ -651,26 +651,16 @@ export class Interpreter<
return;
}
ref._parent = this; // TODO: fix
// If the actor will be stopped right after it's started
// (such as in transient states) don't bother starting the actor.
if (
state.actions.find((otherAction) => {
return (
otherAction.type === actionTypes.stop &&
(otherAction as StopActionObject).params.actor === id
);
})
Comment on lines -657 to -662
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was the main source of the problem because we had actions looking like this:

actions: [stop('a'), invoke('a')]

Stop action for 'a' was simply found in the array and thus the actor was never reinstantiated - despite of the fact that the invoke was coming after it.

) {
// If the actor didn't end up being in the state
// (eg. going through transient states could stop it) don't bother starting the actor.
if (!this.state.children[id]) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since state.children are mutated in setChildren "in order" it should be enough to just check if the final result contains the given id or not.

return;
}
try {
if (autoForward) {
this.forwardTo.add(id);
}

// TODO: determine how this can be immutably updated
this.state.children[id] = ref;

ref.start?.();
} catch (err) {
this.send(error(id, err));
Expand All @@ -679,11 +669,9 @@ export class Interpreter<
},
[actionTypes.stop]: (_ctx, _e, { action }) => {
const { actor } = (action as StopActionObject).params;
const actorRef =
typeof actor === 'string' ? this.state.children[actor] : actor;

if (actorRef) {
this.stopChild(actorRef.name);
if (actor) {
this.stopChild(actor);
}
},
[actionTypes.log]: (_ctx, _e, { action }) => {
Expand Down Expand Up @@ -746,16 +734,8 @@ export class Interpreter<
return undefined;
}

private stopChild(childId: string): void {
const child = this.state.children[childId];
if (!child) {
return;
}

this.forwardTo.delete(childId);
// TODO: determine how this can be immutably updated
delete this.state.children[childId];

private stopChild(child: ActorRef<any, any>): void {
this.forwardTo.delete(child.name);
if (isFunction(child.stop)) {
child.stop();
}
Expand Down
22 changes: 17 additions & 5 deletions packages/core/src/stateUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import {
BaseActionObject,
EventObject,
InvokeActionObject,
StopActionObject,
StateValue,
TransitionConfig,
TransitionDefinition,
Expand Down Expand Up @@ -1588,8 +1589,6 @@ export function resolveMicroTransition<
return inertState as any;
}

const children = { ...currentState.children };

const resolvedConfiguration = willTransition
? Array.from(resolved.configuration)
: !currentState._initial
Expand All @@ -1605,6 +1604,9 @@ export function resolveMicroTransition<

const { context, actions: nonRaisedActions } = resolved;

const children = { ...currentState.children };
setChildren(children, nonRaisedActions);

const nextState = machine.createState({
value: getStateValue(machine.root, resolved.configuration),
context,
Expand All @@ -1627,7 +1629,14 @@ export function resolveMicroTransition<
context !== currentState.context;
nextState._internalQueue = resolved.internalQueue;

nextState.actions.forEach((action) => {
return nextState;
}

export function setChildren<
TContext extends MachineContext,
TEvent extends EventObject
>(children: State<TContext, TEvent>['children'], actions: BaseActionObject[]) {
actions.forEach((action) => {
if (
action.type === actionTypes.invoke &&
(action as InvokeActionObject).params.ref
Expand All @@ -1636,10 +1645,13 @@ export function resolveMicroTransition<
if (ref) {
children[ref.name] = ref;
}
} else if (action.type === actionTypes.stop) {
const ref = (action as StopActionObject).params.actor;
if (ref) {
delete children[ref.name];
}
}
});

return nextState;
}

function resolveActionsAndContext<
Expand Down
2 changes: 1 addition & 1 deletion packages/core/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1209,7 +1209,7 @@ export interface DynamicStopActionObject<
export interface StopActionObject {
type: ActionTypes.Stop;
params: {
actor: string | ActorRef<any>;
actor: ActorRef<any>;
};
}

Expand Down
29 changes: 29 additions & 0 deletions packages/core/test/invoke.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3117,6 +3117,35 @@ describe('invoke', () => {
done();
}, 100);
});

it('should get reinstantiated after reentering the invoking state in a microstep', () => {
let invokeCount = 0;

const machine = createMachine({
initial: 'a',
states: {
a: {
invoke: {
src: () =>
fromCallback(() => {
invokeCount++;
})
},
on: {
GO_AWAY_AND_REENTER: 'b'
}
},
b: {
always: 'a'
}
}
});
const service = interpret(machine).start();

service.send({ type: 'GO_AWAY_AND_REENTER' });

expect(invokeCount).toBe(2);
});
});

describe('actors option', () => {
Expand Down