Skip to content

Conversation

@aloker
Copy link
Contributor

@aloker aloker commented Nov 18, 2018

Fixes the issue #3654

if (handlersExecutedCounter == eventsToPersist.Count)
{
base.ApplyState(nextState.Using(nextData));
nextState = nextState.Using(nextData);
Copy link
Member

@ismaelhamed ismaelhamed Nov 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this a change in behavior, since nextState is actually set in the MakeTransition method? Looking at the JVM, what we're missing is:

base.ApplyState(nextState.Copy(stateData: nextData));

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I must be missing something. Here's what's happening now in ApplyStateOnLastHandler which is called after the persistence is completed.

void ApplyStateOnLastHandler()
{
handlersExecutedCounter++;
if (handlersExecutedCounter == eventsToPersist.Count)
{
base.ApplyState(nextState.Using(nextData));
CurrentStateTimeout = nextState.Timeout;
nextState.AfterTransitionDo?.Invoke(nextState.StateData);
if (doSnapshot)
{
Log.Info($"Saving snapshot, sequence number [{SnapshotSequenceNr}]");
SaveStateSnapshot();
}
}
}

With my comments:

 void ApplyStateOnLastHandler()
{
    handlersExecutedCounter++;
    if (handlersExecutedCounter == eventsToPersist.Count)
    {
        // Using creates a COPY of nextState with stateData set to nextData
        // nextState still refers to the OLD version WITHOUT the NEW nextData being set
        base.ApplyState(nextState.Using(nextData));

        // uses OLD nextState, but that doesn't matter, as Timeout hasn't changed        
        CurrentStateTimeout = nextState.Timeout;

        // HERE's the issue and the actual bug
        // The "AfterTransitionDo" callback (ie, AndThen) - if present - is
        // called with the StateData Property of the OLD, UNCHANGED state.
        // That is, this StateData is always lagging one version behind.        
        nextState.AfterTransitionDo?.Invoke(nextState.StateData);

        // the rest is not relevant
        if (doSnapshot)
        {
            Log.Info($"Saving snapshot, sequence number [{SnapshotSequenceNr}]");
            SaveStateSnapshot();
        }
    }
}

This issue was not discovered using the current set of unit tests, but it is very easily to show (eg. in the shopping cart example) that the current version is broken.

Please try it for yourself. Here's a snippet from the specs:

When(LookingAround.Instance, (evt, state) =>
{
    // NB: When in this state, the cart is empty
    if (evt.FsmEvent is AddItem addItem)
    {
        // visitor adds an item to the EMPTY cart
        return GoTo(Shopping.Instance)
            .Applying(new ItemAdded(addItem.Item))
            .ForMax(TimeSpan.FromSeconds(1))
            .AndThen(cart => /* what to you expect cart to be? Empty or non-empty? */);
    }
    [...]
});

The issue is at the "/* what to you expect cart to be? Empty or non-empty? */" part. You'd expect the cart to be non-empty, but it isn't. It's still the OLD, EMPTY version.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I checked, and line #136 needs to be fixed too:

nextState.AfterTransitionDo?.Invoke(StateData);

With these two changes, it works as expected.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should actually work without. To me, it's either-or. My PR is one solution. The other one is nextState.AfterTransitionDo?.Invoke(nextData); which fixes the issue, too, but for my personal taste is the less clean approach. Ymmv.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, the jvm version uses the second approach. I'll stick to that and will update the PR asap.

@ismaelhamed
Copy link
Member

ismaelhamed commented Nov 24, 2018

Actually @aloker , the whole thing seems to be documented in this issue, so IMO it'd be great if you could bring this PR too, and also check that we're not missing any tests for the PersistentFMS while you're at it ;)

cc: @Aaronontheweb @Horusiath @Danthar

@Danthar
Copy link
Member

Danthar commented Nov 24, 2018 via email

@aloker
Copy link
Contributor Author

aloker commented Nov 24, 2018

@ismaelhamed I'll have a look at it. Thanks for your time!

@Aaronontheweb
Copy link
Member

👍 thanks for the review @ismaelhamed - when this is up to date, I'd be happy to review it for merge

@Aaronontheweb
Copy link
Member

@aloker what's the latest on this?

@aloker
Copy link
Contributor Author

aloker commented Dec 15, 2018

Sorry for the delay. My daily work took all of my time. I have the rest of the year off so I'll be able to take care of this next week.

@Aaronontheweb
Copy link
Member

@aloker I'd be happy to merge this in once the PR is updated - or you and @ismaelhamed resolve your conversation from earlier. Sound good?

@ismaelhamed
Copy link
Member

@aloker Is there any chance of this making it into v1.4? 😉

@Aaronontheweb
Copy link
Member

@ismaelhamed yep - let me re-run CI and check the results

@Aaronontheweb
Copy link
Member

@ismaelhamed is this change good as-is? Or do we need to make some additional updates to it?

@ismaelhamed
Copy link
Member

@Aaronontheweb I think there're a couple of changes still pending

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 this pull request may close these issues.

4 participants