Internal transitions in nested state seems to trigger multiple actions

[hrosa]

Hi,

I've started to use Squirrel recently to build an FSM for some media operations as defined here. So far, the framework seems to have a lot of nice features and is pretty flexible. Great job!

I think I came across a bug though. To implement the PlayCollect feature, I need Play and Collect states to execute in parallel. Like this:

This is how the FSM is setup:

this.builder.defineFinishEvent(PlayRecordEvent.EVALUATE);

        this.builder.onEntry(PlayRecordState.READY).callMethod("enterReady");
        this.builder.transition().from(PlayRecordState.READY).to(PlayRecordState.ACTIVE).on(PlayRecordEvent.START);
        this.builder.onExit(PlayRecordState.READY).callMethod("exitReady");

        this.builder.onEntry(PlayRecordState.ACTIVE).callMethod("enterActive");
        this.builder.transition().from(PlayRecordState.ACTIVE).to(PlayRecordState.READY).on(PlayRecordEvent.EVALUATE);
        this.builder.transition().from(PlayRecordState.ACTIVE).toFinal(PlayRecordState.TIMED_OUT).on(PlayRecordEvent.TIMEOUT);
        this.builder.onExit(PlayRecordState.ACTIVE).callMethod("exitActive");
        this.builder.defineParallelStatesOn(PlayRecordState.ACTIVE, PlayRecordState.COLLECT, PlayRecordState.PROMPT);

        this.builder.defineSequentialStatesOn(PlayRecordState.PROMPT, PlayRecordState._PROMPTING, PlayRecordState._PROMPTED);
        this.builder.onEntry(PlayRecordState._PROMPTING).callMethod("enterPrompting");
        this.builder.internalTransition().within(PlayRecordState._PROMPTING).on(PlayRecordEvent.NEXT_TRACK).callMethod("onPrompting");
        this.builder.transition().from(PlayRecordState._PROMPTING).toFinal(PlayRecordState._PROMPTED).on(PlayRecordEvent.PROMPT_END);
        this.builder.onExit(PlayRecordState._PROMPTING).callMethod("exitPrompting");

        this.builder.onEntry(PlayRecordState._PROMPTED).callMethod("enterPrompted");
        this.builder.onExit(PlayRecordState._PROMPTED).callMethod("exitPrompted");

        this.builder.defineSequentialStatesOn(PlayRecordState.COLLECT, PlayRecordState._COLLECTING, PlayRecordState._COLLECTED);
        this.builder.onEntry(PlayRecordState._COLLECTING).callMethod("enterCollecting");
        this.builder.internalTransition().within(PlayRecordState._COLLECTING).on(PlayRecordEvent.DTMF_TONE).callMethod("onCollecting");
        this.builder.transition().from(PlayRecordState._COLLECTING).toFinal(PlayRecordState._COLLECTED).on(PlayRecordEvent.END_COLLECT);
        this.builder.onExit(PlayRecordState._COLLECTING).callMethod("exitCollecting");

        this.builder.onEntry(PlayRecordState._COLLECTED).callMethod("enterCollected");
        this.builder.onExit(PlayRecordState._COLLECTED).callMethod("exitCollected");

        this.builder.onEntry(PlayRecordState.TIMED_OUT).callMethod("enterTimedOut");

The problem I came across is related to internal state transition in nested states. Like PROMPTING -> Prompting on NEXT_TRACK. It seems to trigger more actions than intended.

Here is a simple test case:

    @Test
    public void testPromptCollect() {
        // given
        final MgcpEventSubject mgcpEventSubject = mock(MgcpEventSubject.class);
        final Recorder recorder = mock(Recorder.class);
        final RecorderListener recorderListener = mock(RecorderListener.class);
        final DtmfDetector detector = mock(DtmfDetector.class);
        final DtmfDetectorListener detectorListener = mock(DtmfDetectorListener.class);
        final Player player = mock(Player.class);
        final PlayerListener playerListener = mock(PlayerListener.class);
        final PlayRecordContext context = new PlayRecordContext();
        final PlayRecordFsm fsm = PlayRecordFsmBuilder.INSTANCE.build(mgcpEventSubject, recorder, recorderListener, detector, detectorListener, player, playerListener, context);

        // when
        fsm.start();
        fsm.fire(PlayRecordEvent.START, context);
        fsm.fire(PlayRecordEvent.NEXT_TRACK, context);
        fsm.fire(PlayRecordEvent.NEXT_TRACK, context);
        fsm.fire(PlayRecordEvent.NEXT_TRACK, context);
        fsm.fire(PlayRecordEvent.NEXT_TRACK, context);
        fsm.fire(PlayRecordEvent.END_COLLECT, context);
        fsm.fire(PlayRecordEvent.PROMPT_END, context);
    }

You can find the complete log here squirrel-internal-transition-bug.txt.

It seems more actions are triggered the greater the number of local transitions we have.

Can you please look into this and confirm it's a bug?

Thanks in advance!

[hrosa]

@hekailiang I'm willing to contribute with a patch for this issue. Since I'm unfamiliar with code at the moment, could you provide any hints for me to get started?

[hekailiang]

go head, pass all the unit test

[hrosa]

@hekailiang any hints on where I should start looking?

[hrosa]

@hekailiang Was able to find the problem.

This line here https://github.com/hekailiang/squirrel/blob/master/squirrel-foundation/src/main/java/org/squirrelframework/foundation/fsm/impl/StateImpl.java#L390 can create duplicate transitions in the sub-state.

In the following example from debugger we clearly see two duplicate COLLECTING sub-states:

I'm planning to avoiding duplication of entries here: https://github.com/hekailiang/squirrel/blob/master/squirrel-foundation/src/main/java/org/squirrelframework/foundation/fsm/impl/StateMachineDataImpl.java#L174

&& !parallelStatesStore.containsEntry(parentStateId, subStateId)

Does this seem correct to you? Test suite is passing and this solves my issue.

[hrosa]

Created PR