-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Fix Akka.Util.Result
edge case
#7433
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 theTask
argument is not completed.Checklist
For significant changes, please ensure that the following have been completed (delete if not relevant):