Fix State machine sub-regions do not resume from last state after restore from persistence #811

[ShvetsovDV]

Fixes spring-projects/spring-statemachine#811

I think, for reason this problem, we need to fix two bags in AbstractPersistingStateMachineInterceptor:

1. definition of real "childRefs" for orthogonal states Here we need to look at the line 171. I'am think condition "stateMachine.getState().isOrthogonal()" is unnecessary.
2. definition of real identifier of state machine for context The fact is that all changes are saved for the main state machine(with a main state machine id). We need to fix it.

And second part of fix it is edit resetStateMachine in AbstractStateMachine

[chutch]

Hey @ShvetsovDV , thanks for looking at it.

I think there are actually 3 different bugs that are contributing to this issue and needs to be addressed. I'm lately looking into this issue on my project and would like to help to push those fixes through. Maybe we can do it one by one?

Resuming

1. DefaultStateMachineService reset on all regions instead of top one

You did fix it already here. Going through all regions is wrong, because the reset is anyway recursive.

2. Regions are being overwritten upon reset.

Fixed here. So all regions we iterate over are going to be always overwritten by last context.

Persisting

There is also an issue with persisting the context of regions. The context of region overwrites the main context.

Now, I'm not sure if the solution proposed is the correct one. Let me explain what I have observed. I have implemented my own StateMachineRuntimePersister, which also implements StateMachineInterceptor interface. There are 2 methods of our interest there:

void preStateChange(State<S, E> state, Message<E> message, Transition<S, E> transition,
            StateMachine<S, E> stateMachine, StateMachine<S, E> rootStateMachine);

void postStateChange(State<S, E> state, Message<E> message, Transition<S, E> transition,
            StateMachine<S, E> stateMachine, StateMachine<S, E> rootStateMachine);

The OOTB AbstractPersistingStateMachineInterceptor implementation persists mostly in preStateChange method. What I observed is that for the postStateChange method, stateMachine and rootStateMachine for regions are always correct and different objects.

However, for preStateChange the stateMachine and rootStateMachine are always (or almost always, I don't recall) the same object. Meaning there is a bug imho somewhere upstream.

Now the "Persisting" issues can be quickly fixed by using custom persister and persisting on postStateChange only. However Resuming issues need to be fixed in the library/project itself.

So, I have two questions... do you think we can split the fixes for persisting & resuming? How can I help to push these things through? I can gladly take over.

[ShvetsovDV]

@chutch, thanks for the answer!

Indeed, when solving the problem, several errors were fixed in different parts of the code. I think we need to commit the edits with one commit, otherwise there will be no positive effect.

I would also like to receive a comment on my commits from @jvalkeal .

However, for preStateChange the stateMachine and rootStateMachine are always (or almost always, I don't recall) the same object. Meaning there is a bug imho somewhere upstream.

Here fixed a bug in determining the machine ID for the context before saving it. Perhaps your effects are due to the fact that buildStateMachineContext is called not only in preStateChange and postStateChange.

I would be glad to receive examples of cases where my solution does not work.

[ShvetsovDV]

OK, I'll try to rephrase to explain my concern. Let's assume that we have following simple State Machine:

            ┌─────►X1─────►X2────┐
            │                    │
start ───►A─┤                   ┌┘►B────►Z
            │                   │
            └─────►Y1─────►Y2───┘

Where:

• start, A, B, Z are states of the root State Machine, with id example
• X1, X2 are states of a sub-machine, a region called x therefore id example#x
• Y1, Y2 is state of another sub-machine, a region called y

Now, let's assume state change from X1 -> X2. From the debugger I see that when postStateChange & preStateChange are called for the same state change, they are called with different parameters

preStateChange

public void preStateChange(State<S, E> state, Message<E> message, Transition<S, E> transition, StateMachine<S, E> stateMachine, StateMachine<S, E> rootStateMachine) {

// with "values"
state=X2
stateMachine=example
rootStateMachine=example

// the very same SM object, root one
stateMachine == rootStateMachine 

postStateChange

public void postStateChange(State<S, E> state, Message<E> message, Transition<S, E> transition, StateMachine<S, E> stateMachine, StateMachine<S, E> rootStateMachine) {

// with "values"
state=X2
stateMachine=example#x  <== correct SM, because the state change happened within a region
rootStateMachine=example

// different SM objects
stateMachine != rootStateMachine 

That makes me think, that maybe trying to fix this issue by "reverse-engineering" with findSmIdByRegion is not a proper way to fix it.

For some reason for the very same state change postStateChange method receives proper SM objects. Then passing those 2 objects to buildStateMachineContext() methods works perfectly fine without any changes in the code. So persisting on postStateChange seems to be working fine.

Whereas preStateChange receive 2x root SM, the very same object, the very same reference.

So this makes me think that maybe the fix should be performed somewhere deeper? Please mind this is just a hunch and maybe I am wrong. I think I would a lot more time to understand the inner workings of SM to see where the bug is. I am sure your code is working and doing the job. I am just wondering if it is not going to seal a deeper issue.

Let me know what you think about it. Hope this makes sense.

You are right that the error goes somewhere deeper. But the source code is difficult to write and needs a serious rewrite. Such elaboration takes time. To carry out serious rewrite without coordination and explanation of certain decisions by the owner of the source code, is fraught with new errors and loss of time.

In my implementation, I am locally parsing the structure of the statemachine in the findSmIdByRegion method. This fix will not introduce new bugs, although it looks pretty crutch.

[ShvetsovDV]

In a serious revision, to simplify the code, I would like methods for analyzing the structure of the state machine similar to findSmIdByRegion (may be findNextState, getAvailableStates, findEventsFromSourceState, ...) into a separate static class.

[kiran-mallineni]

Hey @ShvetsovDV , thanks for looking at it.

I think there are actually 3 different bugs that are contributing to this issue and needs to be addressed. I'm lately looking into this issue on my project and would like to help to push those fixes through. Maybe we can do it one by one?

Resuming

1. DefaultStateMachineService reset on all regions instead of top one

You did fix it already here. Going through all regions is wrong, because the reset is anyway recursive.

2. Regions are being overwritten upon reset.

Fixed here. So all regions we iterate over are going to be always overwritten by last context.

Persisting

There is also an issue with persisting the context of regions. The context of region overwrites the main context.

Now, I'm not sure if the solution proposed is the correct one. Let me explain what I have observed. I have implemented my own StateMachineRuntimePersister, which also implements StateMachineInterceptor interface. There are 2 methods of our interest there:

void preStateChange(State<S, E> state, Message<E> message, Transition<S, E> transition,
          StateMachine<S, E> stateMachine, StateMachine<S, E> rootStateMachine);

void postStateChange(State<S, E> state, Message<E> message, Transition<S, E> transition,
          StateMachine<S, E> stateMachine, StateMachine<S, E> rootStateMachine);

The OOTB AbstractPersistingStateMachineInterceptor implementation persists mostly in preStateChange method. What I observed is that for the postStateChange method, stateMachine and rootStateMachine for regions are always correct and different objects.

However, for preStateChange the stateMachine and rootStateMachine are always (or almost always, I don't recall) the same object. Meaning there is a bug imho somewhere upstream.

Now the "Persisting" issues can be quickly fixed by using custom persister and persisting on postStateChange only. However Resuming issues need to be fixed in the library/project itself.

So, I have two questions... do you think we can split the fixes for persisting & resuming? How can I help to push these things through? I can gladly take over.

yes there is a bug AbstractStateMachine which is calling AbstractPersistingStateMachineInterceptor with incorrect state machine. can we prioritise this to fix in the library? we fixed some parts by extending the library. however AbstractStateMachine is so complicated as there are no proper extension points