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

Stopping machines #185

Closed
kybarg opened this issue Jan 9, 2023 · 10 comments · Fixed by #223
Closed

Stopping machines #185

kybarg opened this issue Jan 9, 2023 · 10 comments · Fixed by #223

Comments

@kybarg
Copy link
Contributor

kybarg commented Jan 9, 2023

Issue

Seems like interpret returns only a referense to origincal service.
So after trying to remove service or referense, service keeps running.

Reproduction

https://codesandbox.io/s/awesome-thunder-rk87mv?file=/src/index.js

@kybarg kybarg changed the title **CRITICAL BUG** **CRITICAL BUG** Interpret reference Jan 9, 2023
@matthewp
Copy link
Owner

matthewp commented Jan 9, 2023

I don't think I understand the bug. You send 2 events to the service which puts it into the child machine. You then create a new service. That doesn't stop the in-flight setTimeout from happening.

Creating a new service doesn't "stop" the existing service. There isn't presently a way to stop a service that's already running, aside from not sending events into it any more.

@matthewp
Copy link
Owner

matthewp commented Jan 9, 2023

A service.stop() would be a good feature to add though.

@kybarg
Copy link
Contributor Author

kybarg commented Jan 10, 2023

@matthewp any suggestions how this can be achieved?
And also don't you find this wrong that reference can be removed and you loose control over state machine, but service is still running?

@matthewp
Copy link
Owner

Add a stop() function here: https://github.com/matthewp/robot/blob/main/machine.js#L189 that probably just sets a this.stopped = true. Then probably send() checks if the service is stopped at the top of the function and exits if so: https://github.com/matthewp/robot/blob/main/machine.js#L169


No, I don't think this is wrong, that's just how JavaScript works. If you don't keep a reference to an object that does stuff... it's going to keep doing stuff.

I do think your example is not something you'd want to happen! That's why I suggested a stop().

However, even with a stop() it's not going to prevent the Promise from resolving. So if there are any mutations inside of the setTimeout handler those are still going to happen. Nothing we can do about that. But we can avoid the machine from continuing to change.

@kybarg
Copy link
Contributor Author

kybarg commented Jan 10, 2023

@matthewp maybe you are right.
Im my appp I use state machine as session manager. So sometime people leave in the middle of the session and it is destoyed (at least I thought it was the case) after timeout. Now it is a memory leak in my case😂
Another option was to implement "terninate" transition for each state, but my app is too big to do so.

@ehuelsmann
Copy link
Collaborator

@matthewp , since this is expected behaviour, would it be an idea to remove the **critical bug** prefix from the subject of the issue?

I'm contemplating the addition of .stop(). It might suggest that the actions in the FSM are going to be cancelled, which isn't the case. Especially long-running functions using async/await will still be able to result in unexpected side effects.

My current thinking is that long async/await functions probably warrant explicit steps in the state machine, though, where the side-effects of each step in the state machine could be cancelled by moving it into a "cancelled" state before the promise resolves, effectively cancelling all further (side-effect-full) processing.

@matthewp
Copy link
Owner

matthewp commented Dec 7, 2024

Yes that's my hesitancy to do something here, we can't actually stop in flight stuff. We can let the machine know not to proceed or trigger actions though

@matthewp matthewp changed the title **CRITICAL BUG** Interpret reference Stopping machines Dec 7, 2024
@ehuelsmann
Copy link
Collaborator

Last week, I tried the pattern I suggested: I had an async function which used await at three points. I converted the execution flow to a workflow, moving the state from the single function to the workflow context. This meant I had to merge the return values of the async calls into the workflow before I could proceed to the next step. The next step is then a function with "regular" execution, ending in either an async call (of which the value is returned by await-ing it) or plainly returning a value.

The workflow invoke does not only have the regular 2 events (done and error), but also a cancel event. The event can be executed while the async call is in-flight. This cancels any processing based on the resolved future (but not the future itself). Earlier fixes prevent the 'done' or 'error' events from being triggered; that is: the workflow has to be in exactly the state in which the async function was triggered in order to consume the 'done' or 'error' events.

@ehuelsmann
Copy link
Collaborator

For background: ledgersmb/LedgerSMB@0418711

@matthewp
Copy link
Owner

Yes, I think we don't document well enough that invoke can have extra events like that. It's just a regular state and can take abitrary events that you define. A cancel event is a great way to do this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants