-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Add and use ConfigureAwaitOptions #87067
Merged
Merged
Changes from 2 commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Just trying to understand this more: isn't this semantically different than the previous code? Specifically: the previous code would catch the exception if the task failed, which caused it to be marked as observed. The new code instead would just never observe the exception if the task failed, as it's just returning the result from the task if it did complete successfully. Wouldn't this make it so that if the task fails, the exception wrapper would not get its finalizer suppressed, and that exception would end up being signaled from the
TaskScheduler.UnobservedTaskException
event, which wasn't the case before this change? 🤔Is that just by design, or am I missing something that makes this not the case here?
Thank you! 🙂
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.
runtime/src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/TaskAwaiter.cs
Line 121 in da1da02
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.
Ooh I had missed that! Will definitely have to keep in mind that for cases where you want to await without throwing but also flow any exceptions to
TaskScheduler.UnobservedTaskException
(eg. we're doing this in the MVVM Toolkit), this new configurable task awaiter shouldn't be used then. Thank you for the additional info! 😄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.
In what situation would you want that? From my perspective, the event is there for cases where the dev simply didn't pay attention. Any gesture that says "I know what I'm doing" supresses it. Just because someone accesses the Exception property or allows the exception to be thrown doesn't mean they've actually reacted to the error... they could have done
_ = t.Exception;
orcatch {}
. I seeSuppressThrowing
as a similar gesture.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.
It's used under one of the configurable options for the
[RelayCommand]
generator (see docs here). Consider:This will cause the generated command to directly await the method. If it throws, it throws. This is the default behavior and matches the usual developer expectation. If you don't handle exceptions, your app goes down. In most cases, you'd just use this, manually add a
try/catch
, and do all the exception handling/logging yourself from there.This will instead await the method by not throwing exceptions, which will instead just flow to
TaskScheduler.UnobservedTaskException
, but without crashing the app. This can be useful in cases where you eg. have some centralized logging system for exceptions from your entire app, from a handler to that event, and where you don't particularly care about a given command to fail (or at least, you don't want that to crash your app).It's just to give developers a bit more flexibility. We've had several feedbacks from people using/liking this option 🙂
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 surprised you want to use UnobservedTaskException for that. The infrastructure is obviously responsible for consuming the task in this case and should also be responsible for dealing with any failures and routing them appropriately. What do you do if the exception is thrown synchronously out of such a Task-returning method, or if it's not Task-returning at all, or if it's a ValueTask backed by an IValueTaskSource that won't trigger UnobservedTaskException? async void methods, for example, queue exceptions to be rethrown on the original SynchronizationContext so that the app model's unhandled exception event can treat those the same as anything else unhandled by an event handler.
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.
The command is just wrapping your async method, so the only other scenario where invoking that command might throw is if you throw from an event handler subscribed to one of the properties of the command that send a notification. But in that case a crash is expected, as it's the same exact behavior you'd get if any other of your handlers (say, for some property change elsewhere) was to just throw while running. It'd be weird if that didn't crash the app.
You can only use
[RelayCommand]
for methods that return aTask
(orvoid
). If you tried to use it over a method that returned something else (like aValueTask
), the generator would just issue a diagnostic producing an error.If a user had an
async void
method with[RelayCommand]
, that's just an error on their side, and if that method threw an exception there would be nothing we could do. But then again, this would be exactly the same behavior you'd get for any other event handler beingasync void
in your app. One of the points of[RelayCommand]
supportingTask
-returning methods is specifically so that it can correctly handle exceptions, as well as offering other useful functionality on top (eg. concurrency control, running checks, etc.). This actually makes me thing we might want to introduce a new analyzer to warn onasync void
methods with[RelayCommand]
... I've seen a few of these in the wild 🤔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.
My point about "async void" was simply that it unifies with the exception handling mechanism of the app model, e.g. whether or not this event handler is marked "async", the same behavior results:
I'm surprised you're trying to create a different mechanism. I'm obviously not your end user, but if I was, I'd expect unhandled exceptions to end up in the same place, raising Application.ThreadException in WinForms, raising Dispatcher.UnhandledException in WPF, etc. UnobservedTaskException was not introduced to be a general exception handling choke point; it was introduced as a back stop for places where you inadvertently neglected to wait on a task. Since you're raising this case as an example of where you'd need to be careful with SuppressThrowing, you're obviously awaiting the task, which means you're going out of your way to use UnobservedTaskException as such a general exception handling choke point.
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.
But we're not doing that. Let me clarify. The default mechanism, which is when you just have your
Task
-returning method for your command, will have the command await that task behind the scenes (in what is ultimately anasync void
handler, since it's invoked by the UI viaICommand.Execute(object)
). If that method throws an exception that isn't handled, the exception will bubble up and result in the same as you're showing above. The main difference is that with your (wrapped) method being asynchronous, you get additional benefit on top, such as the command being able to also notify when an operation is running (which is very convenient to bind to eg. a loading ring, or to disable other commands), or to handle concurrency (eg. you can have the command automatically disable itself if it's already running). But with respect to exception handling, it's the same as what you're describing 🙂The difference is only if you opt-in to
FlowExceptionsToTaskScheduler
, in which case yes the behavior is different. But that's just a separate option, disabled by default, which you can just enable in specific cases if you do want that and know what you're doing and what it will result in. In almost all cases, you would just leave it off.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.
That's the "different mechanism" I'm referring to. We never intended TaskScheduler.UnobservedTaskException to be used in this manner, as something folks would opt-in to sending lots of exceptions to. If you're awaiting the task in your infra, and you don't want to allow the exception to bubble up to the app model's mechanism, I'd expect MVVM toolkit to have its own event handler folks can listen to for that use.
Anyway, thanks for the info.