Problem with Reentry in PermitDynamic in 5.14

[Fieslix]

It seems like since 5.15 PermitDynamic is silently not re-entering the state.

    private enum State {
        Ready,
    }

    private enum Trigger {
        Info
    }

    [Test]
    public void DynamicEntry() {
        var stm = new StateMachine<State, Trigger>(State.Ready);
        var entryFromInfo = false;
        var entry = false;
        var config = stm.Configure(State.Ready);
        config.PermitDynamic(Trigger.Info, () => State.Ready);
        config.OnEntry(() => entry = true);
        config.OnEntryFrom(Trigger.Info, x => entryFromInfo = true);
        stm.Fire(Trigger.Info);
        Assert.That(entry, Is.True, "Entered into Ready");
        Assert.That(entryFromInfo, Is.True, "Entered from trigger info");
    }

[DeepakParamkusam]

This is due to change made in #544. State is no longer re-entered unless the trigger is explicitly configured via PermitReentry. PermitDynamic is also affected due to this.

However, if config.PermitReentry(Trigger.Info); is added to above test configuration, the state machine throws InvalidOperationException-

System.InvalidOperationException: 'Multiple permitted exit transitions are configured from state 'Ready' for trigger 'Info'. Guard clauses must be mutually exclusive.'

It appears that it is no longer possible to re-enter a state dynamically, which I believe is a regression.

@mclift , @crozone , @HenningNT Could you take a look?

[mclift]

Thanks for raising this, @Fieslix & @DeepakParamkusam.

As I see it, allowing state re-entry through a dynamic transition should never have been the default behavior as it's inconsistent with the non-dynamic transitions and makes the API confusing. I also agree that there should be a way to allow a dynamic transition to re-enter the source state.

As PermitDynamic and PermitDynamicIf already have 20+ method signatures between them, perhaps an additional parameter with a default value would be appropriate here.

To represent this internally, perhaps there's an argument for replacing the ReentryTriggerBehaviour class with a property on TriggerBehaviour that sets whether reentry is enabled in instances of its implementations.

So we might end up with something like:

config.PermitDynamic(Trigger.X, () => State.A, permitReentry: true);

How does this sound as an approach?

[Fieslix]

This is a difficult design decision. I see Exception handling as another problem, because you don't know if PermitDynamic will try to re-enter the state on creation of the state machine.

Just to make that clear, I'm okay with not reentering the state, but I would expect the Action to execute. Why would I have that transition, just to ignore it. Okay, It's called the on entry from action, but then again I would expect a way to attach an action to that dynamic self transition.

[mclift]

It's been a while since this was opened and there's been less discussion on the subject that I thought there might be.

As I've thought about it more, my opinion has changed toward restoring the original behaviour (allow re-entry), and to make it clear in the xmldoc that this aspect of dynamic transitions differs from their "static" counterparts. This has been driven by a few things: the fact that PermitReentry is distinct from Permit for the static transition configs; the risk to users of Stateless of regressions being introduced by this change in behaviour; and a reluctance to increase the complexity of the API by adding extra configuration to control reentry behaviour. I'm not sure if that last one is justified.

In any case, the test coverage around state reentry is clearly in need to improvement.

I'll try to get a PR out in the next couple of weeks unless someone else beats me to it, and I'm very interested to hear folks' opinions on this in the meantime.

[Fieslix]

While looking into this, I think there is a decent change this issue is not present when using FireAsync. Which probably means, the issue in #544 is still present inside the Async implementation.

[mclift]

@Fieslix thank you. I've updated the PR to addess issues #543 and #565 in the FireAsync implementation, plus unit tests.