Skip to content
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

Akka.Status.Failure.ToString() throws when Cause is null #3162

Closed
HenryTonnison opened this issue Oct 18, 2017 · 4 comments · Fixed by #3220
Closed

Akka.Status.Failure.ToString() throws when Cause is null #3162

HenryTonnison opened this issue Oct 18, 2017 · 4 comments · Fixed by #3220

Comments

@HenryTonnison
Copy link

HenryTonnison commented Oct 18, 2017

Akka.Actor.Futures.Ask(this ICanTell self, object message, TimeSpan? timeout = null)
will cancel the Task if the Timeout is reached.

The cancelled task is then handled by PipeTo
Akka.Actor.PipeToSupport.PipeTo(this Task taskToPipe, ICanTell recipient, IActorRef sender = null, Func<T, object> success = null, Func<Exception, object> failure = null)

if (tresult.IsCanceled || tresult.IsFaulted)
{
recipient.Tell(failure != null ? failure((Exception) tresult.Exception) : (object) new Status.Failure((Exception) tresult.Exception), sender);
}

The new Status.Failure() is created when task is cancelled the Exception is null.
Akka.Actor.Failure.ToString()
{
return "Failure: " + this.Cause.ToString();
}

Then blows up, in akka's own logging.

System.NullReferenceException: Object reference not set to an instance of an object.
at Akka.Actor.Status.Failure.ToString()
at System.Text.StringBuilder.AppendFormatHelper(IFormatProvider provider, String format, ParamsArray args)
at System.String.FormatHelper(IFormatProvider provider, String format, ParamsArray args)
at System.String.Format(IFormatProvider provider, String format, Object[] args)
at Akka.Event.LoggingBus.UnhandledMessageForwarder.ToDebug(UnhandledMessage message)
at Akka.Event.LoggingBus.UnhandledMessageForwarder.Receive(Object message)
at Akka.Actor.ActorBase.AroundReceive(Receive receive, Object message)
at Akka.Actor.ActorCell.ReceiveMessage(Object message)
at Akka.Actor.ActorCell.Invoke(Envelope envelope)


My opinion....

  1. Failure.ToString() handles null.
  2. In PipeTo, if the task is cancelled, fail with a TaskCanceledException
  3. The Ask doesn't cancel a task on timeout but raises a TimeoutException
@Aaronontheweb
Copy link
Member

Failure.ToString() handles null.

Should definitely do that.

In PipeTo, if the task is cancelled, fail with a TaskCanceledException

Should definitely not do that. Remember: this is a message-passing paradigm, We send the Status.Failure1 message to the actor on the other end of the PipeTo because the Task being executed is typically running outside the actor itself. No chance to handle an exception that way.

The Ask doesn't cancel a task on timeout but raises a TimeoutException

Gotta invoke the CancellationToken in order to ensure that any resources deployed where properly collected afterwards.

@HenryTonnison
Copy link
Author

Thanks for the response. Appreciate I’m not fully up to speed with how this is designed to work.

From my point of view it would be nice to know that the task was cancelled other than receiving a Failure with a null cause, it made me think something had failed when it could have been user cancelled. Maybe a status.cancelled() message?

Also if it fails due to a timeout then to explicitly know this would be useful too.

I am more savvy with the RX world where i’d get either OnComplete or TimeoutException, but I appreciate this is different.

@diegofrata
Copy link

diegofrata commented Oct 30, 2017

@Aaronontheweb If the cause is null, how can anyone figure out the reason for the failure? Plus, anyone handling Status.Failure is prone to make the same mistake Akka itself is doing (forgetting to check whether Cause is null). Setting the cause to some exception (TimeoutException preferably) would solve the current issue and also prevent this from happening on other applications relying on Akka.

@HenryTonnison
Copy link
Author

https://github.com/HenryTonnison/akka.net/commit/c2968c701ef90c4da9c4c58739df88a145eaaad4

That is what I was thinking.

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 a pull request may close this issue.

3 participants