FSM: exception in LogTermination changes stopEvent.Reason to Shutdown

[stijnherreman]

When stopping an FSM with return Stop(new Failure(null)), at first sight it seems to work. But this actually causes an exception in LogTermination(Reason reason) at

akka.net/src/core/Akka/Actor/FSM.cs

Line 1353 in 41c116d

_log.Error(failure.Cause.ToString());

Later on in OnTermination, the type of stopEvent.Reason will be Shutdown instead of Failure.

I'd expect one of these to happen:

• an ArgumentNullException is thrown when calling new Failure(null)
• exceptions in LogTermination(...) are caught so the original stop reason is preserved

[eaba]

@Aaronontheweb I have investigated this issue on this branch and here is the fix attempt:

private void Terminate(State<TState, TData> upcomingState)
{
      if (_currentState.StopReason == null)
      {
          var reason = upcomingState.StopReason;

          //if reason is failure and cause is null, here is the best place to fix it
          //maybe there is a need to create a custom exception for this kind of situation
          if (reason is Failure f && f.Cause is null)
          {
              reason = new Failure(new ArgumentNullException("cause"));
           }
         LogTermination(reason);
         foreach (var t in _timers)
         {
              t.Value.Cancel();
          }
          _timers.Clear();
          _timeoutFuture?.Cancel();
          _currentState = upcomingState;
                
          var stopEvent = new StopEvent<TState, TData>(reason, _currentState.StateName, _currentState.StateData);
           _terminateEvent(stopEvent);
        }
}

[ismaelhamed]

LogTermination should handle the case when Cause is null and don't log. If that is not the desired behavior, LogTermination can always be overridden.

[eaba]

@ismaelhamed ?:

/// <summary>
/// By default, <see cref="Failure"/> is logged at error level and other
/// reason types are not logged. It is possible to override this behavior.
/// Should handle the case when Cause is null and don't log. If that is not the desired behavior, LogTermination can always be overridden
/// </summary>
/// <param name="reason">TBD</param>
protected virtual void LogTermination(Reason reason)
        {
            var failure = reason as Failure;
            if (failure != null)
            {
                if (failure.Cause is Exception)
                {
                    _log.Error(failure.Cause.AsInstanceOf<Exception>(), "terminating due to Failure");
                }
                else if(!(failure.Cause is null))
                {
                    _log.Error(failure.Cause.ToString());
                }
            }
        }

[Arkatufus]

LogTermination should handle the case when Cause is null and don't log. If that is not the desired behavior, LogTermination can always be overridden.

I'd rather have the reverse, LogTermination will always log an error, regardless if Cause is null or not. If this is not the desired behavior, LogTermination can be overriden. It makes the method a lot more consistent with no coding gotchas.

[ismaelhamed]

Agree, it makes more sense. Actually, looks like they just log an empty/null error in the JVM.