Document stopping actors. #1434
Conversation
💥 No ChangesetLatest commit: ce786dd Merging this PR will not cause any packages to be released. If these changes should not cause updates to packages in this repo, this is fine 🙂 If these changes should be published to npm, you need to add a changeset. This PR includes no changesetsWhen 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 |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit ce786dd:
|
| context.childMachine.stop(); | ||
| return actions.stop(context.childMachine.id); |
There was a problem hiding this comment.
I included both versions of stop here because I believe that they are both required.
There was a problem hiding this comment.
I believe that context.childMachine.stop(); should not be needed, but I would have to recheck this. Have you encountered any problems with actions.stop(context.childMachine.id); alone?
There was a problem hiding this comment.
I have not experienced issues, but my WIP machine is not complicated enough to trigger the thing I believe could be an issue. (And I wanted to get a first pass of the documentation together and ask before trying to create a repro of a hypothetical issue.)
My hypothesis comes out of scanning the implementations of these to get a bit more context before filing this PR. The test case that this would need is:
- Parent machine.
- Spawns a child machine (so that it is an
Interpreter, not an actor). - Child machine starts an activity (and stays in that state).
- Child machine's interpreter reference from parent machine's
contexthas a listener attached to it. - Test sending
xstate.stop, callingchildMachine.stop(), or doing both of those things.
Confirm:
- No activities still running. (I think this is what
xstate.stopdoes.) - No listeners still bound. (I think this is what
Interpreter#stopdoes.) - Make sure that it isn't just getting GC'd by way of clobbering all references.
I suppose that if the sent xstate.stop actually bubbled up into something that eventually triggered listener removal on the interpreter then it's good, but in a quick recheck I can't identify where that might happen.
There was a problem hiding this comment.
Okay, I have created the test case:
https://codesandbox.io/s/brave-fermi-nyg8e?file=/src/index.js
My findings:
- Either one of
Interpreter#stoporactions.stop()seem to result in proper teardown. (One of thestopListenerssends asession.stopviaInterpreter#stop.) - What I thought was happening with
activitiesdoesn't appear to be relevant. I'm not sure why.
I also tried to check the behavior for sending actions.stop() as an event to a top-level Interpreter which appears to not work, but I'm not entirely sure what it's doing.
@Andarist Can you also take a look at this test case yourself to make sure that my understanding is correct and that my conclusions are valid?
If my analysis holds, I believe that Interpreter#stop/<actor>.stop() is actually a complete solution.
There was a problem hiding this comment.
@Andarist Can you also take a look at this test case yourself to make sure that my understanding is correct and that my conclusions are valid?
Sure thing! I've forked your demo so it doesn't restart the spawned child all over again - it was easier for me to inspect stuff this way: https://codesandbox.io/s/magical-lamarr-b09k1?file=/src/index.js
Either one of Interpreter#stop or actions.stop() seem to result in proper teardown. (One of the stopListeners sends a session.stop via Interpreter#stop.)
if we are talking about calling those from within that pure action then structures within child get cleaned up nicely but context.childMachine.stop() doesn't clean this child from the parent's structure, whereas actions.pure((context) => actions.stop(context.childMachine.id) }) does both correctly.
I also tried to check the behavior for sending actions.stop() as an event to a top-level Interpreter which appears to not work, but I'm not entirely sure what it's doing.
actions.stop doesn't create an event (well, actually it is compatible with an event interface so it could be abused, sort of, to be sent and processed). It creates an action object - think about actions as of instructions for the interpreter that you'd like something to happen. Those actions are received by the interpreter through other means though than by sending it events so to make service.send(actions.stop()) work you would have to create a transition for the xstate.stop event and create a stop action there. This is not what you are looking for though, I'm just describing this for completeness.
// ALSO: Test the parent behavior.
// service.stop();
This cleans up stuff correctly and I have confirmed that using this demo.
// HELP: Is this correct for trying to send a stop action to an un-parented Interpreter?
// service.send(actions.stop(service.id));
Do you want to stop the service here or some child service of a particular id? If the first one it should be totally valid to just call stop method like this service.stop().
// 2. Check to see if
window.childMachineFirst's StateNode has activities.
It seems that we do not clean children when stopping but they are being stopped. So references are held to them - gonna prepare a fix for this later as semantically it seems right the good thing to do. The child services are being stopped though so GC should kick in most of the time and collect the whole tree of services.
// 3. Check to see if
window.childMachineFirsthas listeners.
This works correctly - those structures are being cleaned up just OK.
There was a problem hiding this comment.
Thank you for doing this, such docs PRs from external contributors are super rare and yet they are very, very valuable, often more valuable than code contributions. Much love for that ❤️
I have left some minor comments, the overall content is very good and very needed for our users. I bet others had similar problems in the past and they couldn't figure out how to do this properly with the documentation alone.
As to the whole stopping thing - yes, pure is required now, but it should be fairly easy to just allow for its dynamic resolution (stop(ctx => ctx.child)) and I think we should implement this unless @davidkpiano has a better idea about how one should manage stopping of the spawned actors.
The PR itself doesn't have to be adjusted to account for those possible changes though - it should describe current possibilities. If we decide to move with the dynamic resolution of stop I will be able to implement it rather quickly and adjust the content of those paragraphs here accordingly.
| @@ -332,6 +332,79 @@ parentService.send('LOCAL.WAKE'); | |||
| // => 'connected' | |||
| ``` | |||
|
|
|||
| ### Stopping Spawned Machines | |||
|
|
|||
| The object returned from a successful `spawn` action is an `Interpreter`. It implements the `actor` interface, but has slightly different behavior. If you call `stop()` on an `Interpreter` it does not clean up any ongoing `activities` in the active `StateNode`. To properly stop spawned machines you must additionally send them an action using `actions.stop()`. | |||
There was a problem hiding this comment.
Is stopping a machine any different than stopping other types of actors/services? In general, in v5 we'll stop exposing Interpreters here at all, all spawned things will just be actors and nothing more than that. It would be best to refrain from v4-specific explanations (if not needed, of course, after all - v4 will still be the latest stable major version for some time).
I would expect the issue to be the same for all types of actors right now as well, as the interpreter doesn't really care about this - it just happens that we do not "wrap" the spawned interpreters as they already implement the actor interface. This leaks the information about the spawned things and gives access to structures that should not be exposed - and that's why we plan to wrap those as just plain actors in the v5.
Right now the actor interface still has the stop method and this is related to the issue you have stumbled upon because how one should stop a spawned service was not documented but at the same time the stop method is super easy to discover, so it was quite natural to just use it.
The underlying reason why stop shouldn't be used in this context is that the spawned actor is "controlled" by the parent and thus they are linked together. So it becomes the responsibility of a parent to manage the lifecycle of the spawned actor - which, again, was not that obvious at all. We need this to clear up references etc. I think it would be best to focus on that high-level aspect here - I think it brings a little bit of a perspective for the reader on why this is needed. I think it should be doable to warn at runtime about incorrect .stop() invocations - but we'd have to look into it. This would certainly improve the overall DX as we could both warn about this operation not being correct and we could point to this section of the documentation for further reading. Could you create an issue about it? We probably won't be able to jump into any time soon, but would still be great to not forget about this.
I would also refrain from mentioning activities here as they will be gone in v5, StateNode is also a class not really mentioned at other places (I think?). Simply mentioning that we need to clean up internal structures should be sufficient.
There was a problem hiding this comment.
Most of this is paragraph is based upon my hypothesis of a possible issue existing. I'll pull together that test case and get back to you on that.
Other notes:
- I will revise to add more of the conceptual model components you describe. That's actually the thing that helps the most from the conversations we've been having in different issue threads. Though I've turned it into a recipe, teaching the conceptual model is probably more important.
- Yeah, got a bit into the weeds.
- I also need to compare this to V5 so I can write this maybe generically enough to apply in both worlds.
| context.childMachine.stop(); | ||
| return actions.stop(context.childMachine.id); |
There was a problem hiding this comment.
I believe that context.childMachine.stop(); should not be needed, but I would have to recheck this. Have you encountered any problems with actions.stop(context.childMachine.id); alone?
|
|
||
| Further, as `assign` actions are run in advance of custom user actions, by the time you would usually attempt to stop a spawned machine, you may need to take care that the reference to the machine in `context` is not overwritten. | ||
|
|
||
| For actions which do not have side effects (such as mutating `context`), you can use `actions.pure()` to ensure that your custom actions is run in order of definition, and before `assign` actions' updates are applied to `context`. Depending upon why you need to stop a spawned machine, this technique may be required. |
There was a problem hiding this comment.
This would actually make a great section of its own - the whole builtin vs custom actions and their ordering. The example itself is also a great explaining why this matters. If you don't have time to move this around or you are unsure where you would fit this - then let us know and we'll figure out something.
There was a problem hiding this comment.
|
Oh, I've missed your questions in the PR comment itself. Let's get to them:
OTOH the This should also answer the second question as those were highly interconnected. |
|
How do y'all prefer your modifications to PRs? And prepping for landing: And landing strategy: And test strategy: (Can also drop this into |
|
Sorry for the delay - was quite busy lately with other stuff. I haven't forgotten and will get back to you this week. |
IMHO - whatever works best for you. Ideally, we'd like to receive often contributions from the community, and requiring a particular git-flow makes it harder to achieve that, not everyone is well-versed with more advanced git techniques etc. As to the landing strategy - this one can actually be quite easily chosen from the GitHub UI so it's not much of a concern either way. |
|
@nathanhammond Sorry for the delay in reviewing this! @Andarist and I discussed the need for a dynamic Thanks so much for doing this! |
|
Now that #1577 is in, we can simplify these docs: Stopping Spawned MachinesTo stop a spawned machine, use the on: {
STOP_SPAWNED: {
actions: actions.stop(context => context.someSpawnedMachine)
}
} |
|
The docs have moved to https://github.com/statelyai/docs and we will be addressing suggestions & improvements there. |
This PR includes two pieces of information that @Andarist explained to me in issue #1403,
actions.pure()andactions.stop().However, after attempting to document (and reading the code in
interpreter.tsfor 4.X), I have additional questions that imply to me that there may be additional unaddressed issues here.xstate.stopto aStateNodedoes not seem to be complete to me as it doesn't seem like it is recursive. The receiver ofxstate.stopmay also be a machine with children which would also need to be stopped. The event does not appear to be propagated. Code..stop()on the interpreter as well. Code.Assuming that my hypotheses are correct, maybe
Interpreter#stopshould also send (from leaf to root?)xstate.stop?