Using PermitIf on a state with substates seems to lead to reentry

[hooligan495]

Hi,

I've a setup where I have a state with Substates.

When I configure the state with a permitif trigger guard, I fire the trigger and the substate transitions.. but if the trigger fires again.. the entry action of the substate I'm already in fires. This seems counter to the documentation I've been reading.

Is this supposed to happen? (I don't want the onentry to be called multiple times). or is this a missunderstanding on my part.

So say I configure a State with a parameterized trigger:

.Configure(SuperStateA).PermitIf(_paramTrigger, SubstateA, p == Event1) .Configure(SubstateA).SubstateOf(SuperStateA).OnEntry(()=>{log.Info("HI)});

If my Event 1 fires multiple times I see Hi printed multiple times.

If this is how you would expect Stateless to behave can someone indicate so? I will update my guard conditions to account for the current state...

Thanks

[HenningNT]

I think that this should not have happened.

If the state machine is in SubstateA, then the trigger should be handled by the super state handler. So far, so good. But the state doesn't change, so the state machine should not execute the onEntry action.

So I would say this is a bug.

[hooligan495]

Thanks

[hooligan495]

@HenningNT in an attempt to be helpful I did create this unit test in the Stateless solution. and implemented the fix this way.. .(I'm not super happy with this fix as I'm not sure it is how you might implement this fix. (for example, in the "findhandler" maybe that should somehow return an "ignoretriggerbehaviour" result. trying to have the result.handler.ResultsInTransitionFrom return false cause the "not trigger defined" exception in the default case. Either of those feel better than what I proposed here but I wanted to start the conversation at least"

If you have other suggestions on how this should be addressed I'd be happy to modify my changes and open a PR. If you like this and would like me to open a PR I'm fine doing that too.

Unit test:

    public void WhenInSubstate_TriggerSuperStateTwiceToSameSubstate_DoesNotReenterSubstate()
    {
        var sm = new StateMachine<State, Trigger>(State.A);
        var eCount = 0;
        sm.Configure(State.B)
            .OnEntry(() => { eCount++;})
            .SubstateOf(State.C);
        sm.Configure(State.A)
            .SubstateOf(State.C);
        sm.Configure(State.C)
            .Permit(Trigger.X, State.B);
        sm.Fire(Trigger.X);
        sm.Fire(Trigger.X);
        Assert.Equal(1, eCount);
    }

code change in statemachine.cs (line 424ish)

             case TransitioningTriggerBehaviour _ when result.Handler.ResultsInTransitionFrom(source, args, out destination):
             {
                 //If a trigger was found on a superstate that would cause unintended reentry, don't trigger.
                 if (source.Equals(destination))
                     break;
                 // Handle transition, and set new state
                 var transition = new Transition(source, destination, trigger, args);
                 HandleTransitioningTrigger(args, representativeState, transition);
                 break;
             }

[hooligan495]

Hi, I'm just checking in on this and my pull request. Is there anything more I can be doing to help move this forward? Thanks!

[HenningNT]

Ping! @crozone @mclift

[crozone]

Ping! @crozone @mclift

Thanks for the ping. LGTM and tests are passing. I'll give @mclift a chance to review this but I think I understand what's going on now. If I understand it, the superstate is transitioning to itself with TransitioningTriggerBehaviour because only the primary substate will use the InternalTriggerBehaviour case which has been set up, so this effectively kills that edge case.

[HenningNT]

I think it's about time to merge in #544, bump the version number to 5.13.1, and make a new release :-)

[mclift]

It's been a little while since the last release. There was a discussion about bumping the major version number as we've changed the targeted frameworks to the LTS versions, but on reflection, this probably doesn't warrant a major release on its own. So maybe 5.14.0?

Anyway, I'll get on that :)