Akka Fsm Update #2559

alexvaluyskiy · 2017-03-16T20:46:07Z

additional debug logging for fsm [2.4.9]
per additional test for onTransition behaviour when initialize [2.4.0]
cancel StateTimeout when FSM actor is stopped [2.4.0]
Send transistion on goto(CurrentState) in FSM [2.4.0]
Make all messages as sealed
forMax(Inf) can now override stateTimeout [2.3.10]
Implement FSMActor_must_unlock_the_lock test
Implement FSMActor_must_log_events_and_transitions_if_asked_to_do_so test
Implement FSMActor_must_fill_rolling_event_log_and_hand_it_out test

alexvaluyskiy · 2017-03-18T15:17:24Z

src/core/Akka/Actor/FSM.cs

-        /// <param name="nextStateName">State designator for the next state</param>
-        /// <param name="stateData">Data for next state</param>
-        /// <returns>State transition descriptor</returns>
+        [Obsolete("GoTo method is osboleted. Use GoTo(TState nextStateName) [1.2.0]")]
        public State<TState, TData> GoTo(TState nextStateName, TData stateData)


This method is not present in the Scala API

I don't think I've even seen this method get used. It's always been GoTo().WithState AFAIK

alexvaluyskiy · 2017-03-18T15:18:14Z

src/core/Akka.Persistence/Fsm/PersistentFSMBase.cs

@@ -1059,7 +1059,7 @@ public State ForMax(TimeSpan timeout)
            /// <summary>
            /// TBD
            /// </summary>
-            public List<object> Replies
+            public IReadOnlyCollection<object> Replies


We should prefer ReadOnly* and Immutable collections in the public API. But this change could break compatibility

It shouldn't really affect how end-users are consuming it that much, unless they were modifying the collection themselves which would have been "illegal" anyway. Guess this falls into the gray area of "is fixing a bug in the public API really a breaking change?" I'm fine with it.

alexvaluyskiy · 2017-03-18T15:18:49Z

src/core/Akka.Tests/Actor/FSMTimingSpec.cs

+        [Fact]
+        public void FSM_must_notify_unhandled_messages()
+        {
+            // EventFilter


EventFilter chaining is not working as expected

I'm assuming that's unrelated to this PR, and IIRC it's an area where we have some known bugs still anyway.

alexvaluyskiy · 2017-03-18T15:19:44Z

src/core/Akka/Actor/FSM.cs

-        public class Shutdown : Reason { }
+        public sealed class Shutdown : Reason
+        {
+            private Shutdown() { }


This one could break compatibility. Probably I should keep a public constructor with ObsoletedAttribute

Same as previous. Go ahead and add that back.

alexvaluyskiy · 2017-03-18T15:19:48Z

src/core/Akka/Actor/FSM.cs

-        public class Normal : Reason { }
+        public sealed class Normal : Reason
+        {
+            private Normal() { }


This one could break compatibility. Probably I should keep a public constructor with ObsoletedAttribute

Yeah, I would do that.

alexvaluyskiy · 2017-03-18T15:20:42Z

src/core/Akka/Actor/FSM.cs

-                if(_debugLog != null)
-                    send = () =>
-                    {
-                        _debugLog.Debug("{0}Timer '{1}' went off. Sending {2} -> {3}",_ref.IsCancellationRequested ? "Cancelled " : "", name, message, actor);


Why this debugging was implemented? It is not present in the scala API

alexvaluyskiy · 2017-03-18T15:21:04Z

src/core/Akka/Actor/FSM.cs

            }

            /// <summary>
            /// TBD
            /// </summary>
            public void Cancel()
            {
-                if (!_ref.IsCancellationRequested)


IsCancellationRequested is always false

alexvaluyskiy · 2017-03-18T15:23:01Z

src/core/Akka/Actor/FSM.cs

-        public class StateTimeout { }
+        public sealed class StateTimeout
+        {
+            private StateTimeout() { }


This one could break compatibility. Probably I should keep a public constructor with ObsoletedAttribute

Same as previous. Go ahead and add that back.

alexvaluyskiy · 2017-03-18T15:53:08Z

Why we have implemented LoggingFsm and an interface with implicit behavior. Instead of creating LoggingFsm class?

alexvaluyskiy · 2017-03-18T17:37:53Z

Should I implement this one?
akka/akka#17152

alexvaluyskiy · 2017-03-18T18:17:01Z

When we are converting scala's traits, we should prefer to use Java API experience. Like to create an additional abstract class, like AbstractLoggingFSM

alexvaluyskiy · 2017-03-18T18:18:23Z

Why we need FSMBase abstract class? All messages should situated inside FSM class. FSMBase is redundant class. But we could not drop it without breaking changes.
At least we could make it static. And FSM actor will inherit ActorBase

Aaronontheweb

Some minor changes requested, but otherwise it looks good.

Aaronontheweb · 2017-03-18T18:12:33Z

src/core/Akka.API.Tests/CoreAPISpec.ApproveCore.approved.txt

@@ -768,52 +769,55 @@ namespace Akka.Actor
    public abstract class FSMBase : Akka.Actor.ActorBase
    {
        protected FSMBase() { }
-        public class CurrentState<TS>
+        public sealed class CurrentState<TS>


Sealing all of this off seems good to me.

Aaronontheweb · 2017-03-18T18:13:26Z

src/core/Akka.API.Tests/CoreAPISpec.ApprovePersistence.approved.txt

@@ -846,14 +846,14 @@ namespace Akka.Persistence.Fsm
        public void WhenUnhandled(Akka.Persistence.Fsm.PersistentFSMBase<TState, TData, TEvent>.StateFunction stateFunction) { }
        public class State<TState, TData, TEvent> : Akka.Actor.FSMBase.State<TState, TData>
        {
-            public State(TState stateName, TData stateData, System.Nullable<System.TimeSpan> timeout = null, Akka.Actor.FSMBase.Reason stopReason = null, System.Collections.Generic.List<object> replies = null, Akka.Util.ILinearSeq<TEvent> domainEvents = null, System.Action<TData> afterTransitionDo = null) { }
+            public State(TState stateName, TData stateData, System.Nullable<System.TimeSpan> timeout = null, Akka.Actor.FSMBase.Reason stopReason = null, System.Collections.Generic.IReadOnlyCollection<object> replies = null, Akka.Util.ILinearSeq<TEvent> domainEvents = null, System.Action<TData> afterTransitionDo = null) { }


Like this change too. We should have been using read-only collections from the start here.

Aaronontheweb · 2017-03-18T18:14:42Z

src/core/Akka.Persistence/Fsm/PersistentFSMBase.cs

@@ -1059,7 +1059,7 @@ public State ForMax(TimeSpan timeout)
            /// <summary>
            /// TBD
            /// </summary>
-            public List<object> Replies
+            public IReadOnlyCollection<object> Replies


It shouldn't really affect how end-users are consuming it that much, unless they were modifying the collection themselves which would have been "illegal" anyway. Guess this falls into the gray area of "is fixing a bug in the public API really a breaking change?" I'm fine with it.

Aaronontheweb · 2017-03-18T18:15:45Z

src/core/Akka.Tests/Actor/FSMTimingSpec.cs

+        [Fact]
+        public void FSM_must_notify_unhandled_messages()
+        {
+            // EventFilter


I'm assuming that's unrelated to this PR, and IIRC it's an area where we have some known bugs still anyway.

Aaronontheweb · 2017-03-18T18:17:03Z

src/core/Akka/Actor/FSM.cs

-        public class Normal : Reason { }
+        public sealed class Normal : Reason
+        {
+            private Normal() { }


Yeah, I would do that.

Aaronontheweb · 2017-03-18T18:19:05Z

src/core/Akka/Actor/FSM.cs

-        /// <param name="nextStateName">State designator for the next state</param>
-        /// <param name="stateData">Data for next state</param>
-        /// <returns>State transition descriptor</returns>
+        [Obsolete("GoTo method is osboleted. Use GoTo(TState nextStateName) [1.2.0]")]
        public State<TState, TData> GoTo(TState nextStateName, TData stateData)


I don't think I've even seen this method get used. It's always been GoTo().WithState AFAIK

Aaronontheweb · 2017-03-18T18:19:49Z

src/core/Akka/Actor/FSM.cs

-            if(_timers.ContainsKey(name))
+            if (DebugEvent)
+            {
+                _log.Debug($"setting {(repeat ? "repeating" : "")} timer {name}/{timeout}: {msg}");


Liking the C#6 API here... makes it way easier to read.

Hold my beer. I'm planning to do C# 7 refactoring :)

Aaronontheweb · 2017-03-18T18:20:41Z

src/core/Akka/Actor/FSM.cs

-                _stateTimeouts.Add(state, timeout);
-            else
-                _stateTimeouts[state] = timeout;
+            _stateTimeouts.AddOrSet(state, timeout);


Change this to

stateTimeouts[state] = timeout

AddOrSet is something stupid I added to the codebase that we should deprecate and remove at some point. It's an unnecessary abstraction.

Aaronontheweb · 2017-03-18T18:21:38Z

src/core/Akka/Actor/FSM.cs

-                })
-                .With<InternalActivateFsmLogging>(_=> { DebugEvent = true; })
-                .Default(msg =>
+                    ProcessMsg(StateTimeout.Instance, "state timeout");


Way easier to read (the main FSM handling routine) and will have fewer allocations also. Nice job.

We should avoid of PatternMatch class

Aaronontheweb · 2017-03-18T18:22:27Z

src/core/Akka/Actor/FSM.cs

-                replies.Reverse();
-                foreach (var r in replies) { Sender.Tell(r); }
-                if (!_currentState.StateName.Equals(upcomingState.StateName))
+                nextState.Replies.Reverse().ForEach(r => Sender.Tell(r));


Much cleaner.

I think I should get it back. List.Reverse has better performance than IEnumerable.Reverse

instead of reversing the list and then iterating, why not do both like

for (int i = nextState.Replies.Count - 1; i >= 0; i--) { Sender.Tell(nextState.Replies[i]); }

Danthar · 2017-03-22T10:29:54Z

src/core/Akka.API.Tests/CoreAPISpec.ApprovePersistence.approved.txt

@@ -854,7 +854,7 @@ namespace Akka.Persistence.Fsm
            public System.Action<TData> AfterTransitionHandler { get; }
            public Akka.Util.ILinearSeq<TEvent> DomainEvents { get; }
            public bool Notifies { get; set; }
-            public System.Collections.Generic.IReadOnlyCollection<object> Replies { get; }
+            public System.Collections.Generic.IReadOnlyList<object> Replies { get; }


Why the switch to IReadOnlyList ?

Because I've implemented this code :)

for (int i = nextState.Replies.Count - 1; i >= 0; i--) { Sender.Tell(nextState.Replies[i]); }

Aaronontheweb · 2017-03-22T16:37:06Z

@alexvaluyskiy on the LoggingFsm bit, because I thought that preferable to having an explosion of base classes to choose from. Better to be able to decorate an existing one to add a piece of minor functionality like that instead of having to change the class you inherit from.

alexvaluyskiy commented Mar 18, 2017

View reviewed changes

alexvaluyskiy changed the title ~~[WIP] Akka Fsm Update~~ Akka Fsm Update Mar 18, 2017

alexvaluyskiy force-pushed the updatefsm branch from f98f770 to 294354b Compare March 18, 2017 16:03

Aaronontheweb requested changes Mar 18, 2017

View reviewed changes

alexvaluyskiy force-pushed the updatefsm branch 3 times, most recently from 2159c5d to a2bd36f Compare March 22, 2017 06:58

fsm update

77a5330

alexvaluyskiy force-pushed the updatefsm branch from a2bd36f to 77a5330 Compare March 22, 2017 06:58

alexvaluyskiy and others added 2 commits March 22, 2017 09:00

changed API to IReadOnlyList

601e039

the latest fix

4ebd0a7

Danthar reviewed Mar 22, 2017

View reviewed changes

Merge branch 'dev' into updatefsm

49ee17f

Aaronontheweb approved these changes Mar 22, 2017

View reviewed changes

Aaronontheweb merged commit 9f33872 into akkadotnet:dev Mar 22, 2017

alexvaluyskiy deleted the updatefsm branch March 22, 2017 19:10

Akka Fsm Update #2559

Akka Fsm Update #2559

Conversation

alexvaluyskiy commented Mar 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexvaluyskiy commented Mar 18, 2017

alexvaluyskiy commented Mar 18, 2017

alexvaluyskiy commented Mar 18, 2017

alexvaluyskiy commented Mar 18, 2017 • edited Loading

Aaronontheweb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexvaluyskiy Mar 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Aaronontheweb commented Mar 22, 2017

alexvaluyskiy commented Mar 16, 2017 •

edited

Loading

alexvaluyskiy commented Mar 18, 2017 •

edited

Loading

alexvaluyskiy Mar 22, 2017 •

edited

Loading