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

Ask does not set exception on the underlying TaskCompletionSource when callee responds with Failure #3091

Closed
ondrejpialek opened this issue Sep 9, 2017 · 2 comments · Fixed by #5187

Comments

@ondrejpialek
Copy link
Contributor

ondrejpialek commented Sep 9, 2017

Hello,

I am running Akka.net 1.3.0 on .NET Core, and came across the following issue:

I am calling Ask<TResult> from outside of the system and would like to have Ask throw an exception at the call site if the underlying actor responds with new Failure() { Exception = ex } instead of TResult.

This behaviour is described in the docs:

To complete the Task with an exception you need send a Failure message to the sender. This is not done automatically when an actor throws an exception while processing a message.

This feature is however not implemented, if you look at

_result.TrySetResult(message);
the FutureActorRef sets the result of the task without inspecting whether the message is a Failure.

This then breaks Ask<TResult> as the underlying extension method tries to cast Failure to TResult.

Unless I am mistaken the following modification could be used:

if (message is Failure failure) {
    _result.TrySetException(failure.Exception ?? new SomeDefaultException())
} else {
    _result.TrySetResult(message);
}

I am assuming this is still a behaviour you want to support. If not then the docs should be updated so that they don't put peoples hopes up :)

Many thanks,
Ondrej.

@ondrejpialek
Copy link
Contributor Author

Upon further inspection of the source code it seems like Status.Failure is used instead internally among Actors to signal an error in processing. If this feature is to be implemented Status.Failure would probably be a better fit.

@marcpiechura
Copy link
Contributor

This will probably be fixed in #3339 , see also #3329 for detailed discussion

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants