-
Notifications
You must be signed in to change notification settings - Fork 1.1k
fix: data provided to AndThen (PersistentFSM) is not the latest version #3655
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| if (handlersExecutedCounter == eventsToPersist.Count) | ||
| { | ||
| base.ApplyState(nextState.Using(nextData)); | ||
| nextState = nextState.Using(nextData); |
There was a problem hiding this comment.
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));There was a problem hiding this comment.
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.
akka.net/src/core/Akka.Persistence/Fsm/PersistentFSM.cs
Lines 129 to 143 in d9f6c57
| 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
+1 from me
Op za 24 nov. 2018 12:57 schreef Ismael Hamed <notifications@github.com:
… Actually @aloker <https://github.com/aloker> , the whole thing seems to
be documented in this issue <akka/akka-core#23739>,
so IMO I'd be great if you could bring this PR
<akka/akka-core#23740> too, and also check that we're
not missing any tests for the PersistentFMS while you're at it ;)
cc: @Aaronontheweb <https://github.com/Aaronontheweb> @Horusiath
<https://github.com/Horusiath> @Danthar <https://github.com/Danthar>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3655 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AA8Nl93X4NN_XUvhvCo_QsKV1R1Dm7GZks5uyTQUgaJpZM4Yn6e3>
.
|
|
@ismaelhamed I'll have a look at it. Thanks for your time! |
|
👍 thanks for the review @ismaelhamed - when this is up to date, I'd be happy to review it for merge |
|
@aloker what's the latest on this? |
|
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. |
|
@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? |
|
@aloker Is there any chance of this making it into v1.4? 😉 |
|
@ismaelhamed yep - let me re-run CI and check the results |
|
@ismaelhamed is this change good as-is? Or do we need to make some additional updates to it? |
|
@Aaronontheweb I think there're a couple of changes still pending |
Fixes the issue #3654