-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Await task extensions fix #2805
Await task extensions fix #2805
Conversation
Change Await to not produce error Callback if there's an exception in the completed callback.
{ | ||
completedCallback?.Invoke(); | ||
} | ||
catch { |
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.
I'm not user I like swallowing the exceptions in the completed callback. If an error happens in the callback, the developer should be notified via an exception. if anything we should probably rethrow the exception
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.
If I re-throw the exception, that would be an exception within an async void method that you're not awaiting, so you wouldn't see it, though it might crash the program.
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.
I could make it a Task and make the calling methods run the task as a separate thread, but then they're still going to not be able to notify of the error.
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.
I'm curious if something like this would be better
bool error = false;
try
{
await task.ConfigureAwait(configureAwait);
}
catch (Exception ex)
{
error = true;
errorCallback?.Invoke(ex);
}
finally
{
if (!error)
completedCallback?.Invoke();
}
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.
@Binderbound did you see my suggestion? Thoughts?
@Binderbound I'm closing this as there has been no activity on it for quite a while now. We welcome your contribution though. If you would like to follow up with the updates that were requested by @brianlagunas we can look at reopening this. |
Description of Change
Changed TaskExtensions / TaskExtensions{T} to not trigger errorCallback if an exception is raised by completedCallback.
Bugs Fixed
#2804
API Changes
None
Behavioral Changes
Await will not trigger errorCallback if there is an error in completedCallback.
PR Checklist