-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Fix Akka.Util.Result edge case
#7433
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
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.
Self-review
| if (task.IsCanceled && task.Exception is null) | ||
| { | ||
| try | ||
| { | ||
| _ = task.GetAwaiter().GetResult(); | ||
| } | ||
| catch(Exception e) | ||
| { | ||
| return new Result<T>(e); | ||
| } | ||
|
|
||
| throw new InvalidOperationException("Should never reach this line!"); | ||
| } |
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.
Fix, detect edge case where a Task is cancelled and its Exception property is null.
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.
LGTM
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.
Self-review
| if(!task.IsCompleted) | ||
| throw new ArgumentException("Task is not completed. Result.FromTask only accepts completed tasks.", nameof(task)); |
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.
New behavior, .FromTask() did not check if the Task argument is completed or not, it should only accept completed tasks
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.
LGTM
Fixes #7432
Changes
Akka.Util.Result<T>.FromTask()checks for edge casesAkka.Util.Result<T>.FromTask()behavior, it will now throw if theTaskargument is not completed.Checklist
For significant changes, please ensure that the following have been completed (delete if not relevant):