-
Notifications
You must be signed in to change notification settings - Fork 4.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
Add and use ConfigureAwaitOptions #87067
Conversation
Note regarding the This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change. |
Tagging subscribers to this area: @dotnet/area-system-threading-tasks Issue DetailsAdds the ability to further control how awaits are performed, with the ConfigureAwaitOptions enum. For .NET 8 this is just for Mostly everything here works as I'd like. The one thing I'm not happy about is this adds an extra branch to the configured awaiter's IsCompleted property that's now incurred on every +xor edx, edx
+mov dword ptr [rbp-08H], edx
+test byte ptr [rbp-08H], 4
+jne SHORT G_M000_IG04
mov rdx, gword ptr [rbp-10H]
test dword ptr [rdx+34H], 0x1600000
jne SHORT G_M000_IG07 On the positive front, I've used the new overload throughout the repo to replace various custom awaiters, use of Task.Run, and places we were catching all exceptions to suppress them with awaits. Some of these just help to clean up the code; others have / enable meaningful perf improvements. For example:
using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;
using System.IO.Pipes;
using System.Threading.Tasks;
using System.Threading;
[MemoryDiagnoser(false)]
public class Program
{
static void Main(string[] args) => BenchmarkSwitcher.FromAssembly(typeof(Program).Assembly).Run(args);
private readonly CancellationTokenSource _cts = new CancellationTokenSource();
private readonly byte[] _buffer = new byte[1];
private AnonymousPipeServerStream _server;
private AnonymousPipeClientStream _client;
[Params(false, true)]
public bool Cancelable { get; set; }
[GlobalSetup]
public void Setup()
{
_server = new AnonymousPipeServerStream(PipeDirection.Out);
_client = new AnonymousPipeClientStream(PipeDirection.In, _server.ClientSafePipeHandle);
}
[GlobalCleanup]
public void Cleanup()
{
_server.Dispose();
_client.Dispose();
}
[Benchmark(OperationsPerInvoke = 1000)]
public async Task ReadWriteAsync()
{
CancellationToken ct = Cancelable ? _cts.Token : default;
for (int i = 0; i < 1000; i++)
{
ValueTask<int> read = _client.ReadAsync(_buffer, ct);
await _server.WriteAsync(_buffer, ct);
await read;
}
}
}
|
57e5bac
to
79fadea
Compare
src/libraries/System.Private.CoreLib/src/System/Threading/Tasks/ConfigureAwaitOptions.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Threading.Tasks/tests/System.Runtime.CompilerServices/TaskAwaiterTests.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Threading.Tasks/tests/System.Runtime.CompilerServices/TaskAwaiterTests.cs
Show resolved
Hide resolved
src/libraries/Common/src/System/Threading/AsyncOverSyncWithIoCancellation.cs
Show resolved
Hide resolved
src/libraries/System.Diagnostics.Process/src/System/Diagnostics/AsyncStreamReader.cs
Show resolved
Hide resolved
79fadea
to
6865088
Compare
Adds the ability to further control how awaits are performed, with the ConfigureAwaitOptions enum. For .NET 8 this is just for `Task` / `Task<TResult>`, and for the latter, the `SuppressThrowing` option isn't supported (as it can make the TResult erroneous). Also uses it throughout the repo to replace various custom awaiters, use of Task.Run, and places we were catching all exceptions to suppress them with awaits. Some of these just help to clean up the code; others have / enable meaningful perf improvements.
6865088
to
74a5fc9
Compare
src/libraries/System.Net.HttpListener/src/System/Net/Managed/HttpResponseStream.Managed.cs
Outdated
Show resolved
Hide resolved
…ttpResponseStream.Managed.cs
/// <summary>No options specified.</summary> | ||
/// <remarks> | ||
/// <see cref="Task.ConfigureAwait(ConfigureAwaitOptions)"/> with a <see cref="None"/> argument behaves | ||
/// identically to using <see cref="Task.ConfigureAwait(bool)"/> with a <see langword="false"/> argument. | ||
/// </remarks> | ||
None = 0x0, |
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.
There used to be a problem with docs.microsoft.com not rendering remarks
on enum constants: dotnet/docs#23950. That issue was closed because it was moved to an internal system, but I don't know whether the bug itself has been fixed. https://github.com/dotnet/dotnet-api-docs/wiki/Remarks does not mention enums. More evidence in dotnet/dotnet-api-docs#5220.
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.
@gewarren, is this still the case? If the docs really can't handle remarks, I'd expect our tooling that seeds the docs from our XML comments to massage them appropriately, or for the doc rendering engine to do so, but not for them to just be ignored. cc: @carlossanlop
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.
Yes unfortunately remarks on enum fields are currently ignored. The internal tracking item is here: https://dev.azure.com/ceapex/Engineering/_workitems/edit/801799
cc @kexugit
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.
@gewarren, thanks, what would you recommend in the interim?
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.
@stephentoub I think the two options are 1) Put the remarks in the summary for the field or 2) Put all the field remarks in the remarks for the enum itself. If option 1) is too heavyweight for IntelliSense, then I'd go with 2).
Task<byte[]?> task = s_downloadBytes(uri, cts?.Token ?? default, async); | ||
await ((Task)task).ConfigureAwait(ConfigureAwaitOptions.SuppressThrowing); | ||
if (task.IsCompletedSuccessfully) | ||
{ | ||
return task.Result; | ||
} |
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
task.MarkExceptionsAsHandled(); |
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;
or catch {}
. I see SuppressThrowing
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:
partial class MyViewModel
{
[RelayCommand]
private async Task DoStuffAsync()
{
}
}
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.
partial class MyViewModel
{
[RelayCommand(FlowExceptionsToTaskScheduler = true)]
private async Task DoStuffAsync()
{
}
}
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.
"What do you do if the exception is thrown synchronously out of such a Task-returning method"
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.
"if it's not
Task
-returning at all, or if it's aValueTask
backed by anIValueTaskSource
that won't triggerUnobservedTaskException
?"
You can only use [RelayCommand]
for methods that return a Task
(or void
). If you tried to use it over a method that returned something else (like a ValueTask
), the generator would just issue a diagnostic producing an error.
"
async void
methods"
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 being async void
in your app. One of the points of [RelayCommand]
supporting Task
-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 on async 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.
"I'm surprised you're trying to create a different mechanism"
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 an async void
handler, since it's invoked by the UI via ICommand.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.
I'm surprised you're trying to create a different mechanism
But we're not doing that. [...] The difference is only if you opt-in to FlowExceptionsToTaskScheduler, in which case yes the behavior is different
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.
{ | ||
CheckIfNetworkChanged(); | ||
} | ||
} | ||
catch (OperationCanceledException) | ||
{ | ||
} | ||
finally | ||
{ | ||
timer.Dispose(); |
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.
Why is an explicit try-finally
+ Dispose()
better here?
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 don't know whether this is significant, but calling Dispose
from the finally
clause of the existing try
…catch
results in smaller IL than letting a using
statement expand to an outer try
…finally
. SharpLab:
code size | MoveNext | SetStateMachine | entry |
---|---|---|---|
PeriodicallyCheckIfNetworkChanged | 363 (0x16b) | 13 (0xd) | 55 (0x37) |
PeriodicallyCheckIfNetworkChangedUsingStatement | 374 (0x176) | 13 (0xd) | 55 (0x37) |
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.
At the IL level both will have two separate try
blocks. The size difference seems to come from Roslyn adding a null check before calling Dispose
(which it could elide by doing some basic code analysis) and also adding some weird useless code at the beginning of the try block (no idea where that comes from):
// sequence point: hidden
IL_0086: ldloc.0
IL_0087: ldc.i4.1
IL_0088: pop
IL_0089: pop
// sequence point: hidden
IL_008a: nop
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 is slightly smaller, but I changed it mainly because I was already editing this method, and I find fewer such constructs in the source easier to reason about. A single try/catch/finally vs a using around a try/catch. It's not a big deal either way.
Hi @stephentoub - TLDR
because I'm trying to use the return value. And this should not:
Because I do not actually try to retrieve the invalid result. This will be useful because it does not force me to try/catch simply because a method I'm calling has a return value that I'm not using. |
@TonyValenti, |
My comment was that the throw should happen not on ConfigureAwait but when the .Result is accessed. |
|
I don't see in 12.9.8.4 Run-time evaluation of await expressions any way for the awaiter to know whether the result of |
Correct |
What would the below code print? Or would it throw? async Task<string> WorkAsync() { throw new NotImpelementedException(); }
var value = await WorkAsync().ConfigureAwait(ConfigureAwaitOptions.SuppressThrowing);
Console.WriteLine(value); |
With: var value = await WorkAsync().ConfigureAwait(ConfigureAwaitOptions.SuppressThrowing); the call to ConfigureAwait would throw an ArgumentOutOfRangeException. There's also an analyzer out for PR that will result in that being a build warning. With: var value = await ((Task)WorkAsync()).ConfigureAwait(ConfigureAwaitOptions.SuppressThrowing); it wouldn't compile because the expression is void. |
Thanks! 🙂 |
Adds the ability to further control how awaits are performed, with the ConfigureAwaitOptions enum. For .NET 8 this is just for
Task
/Task<TResult>
, and for the latter, theSuppressThrowing
option isn't supported (as it can make the TResult erroneous).Mostly everything here works as I'd like. The one thing I'm not happy about is this adds an extra branch to the configured awaiter's IsCompleted property that's now incurred on every
await task.ConfigureAwait(false)
. That branch should be elidable by the JIT, as the input is invariably constant (false
) and it should be able to then see that constant value through to the IsCompleted call immediately after the awaiter is constructed. However, because in the async method the awaiter is later passed by ref to the method builder, it's considered address exposed, the struct won't be promoted, and the JIT loses track of the known constant value, such that even though everything is inlined, it still does the(options & ForceYielding) != 0
check in IsCompleted. I'm talking with @EgorBo about whether there are any options here. It's not worth introducing a whole new set of awaitables/awaiters for this, but hopefully we can come up with a way in the existing ones to remove the few additional instructions incurred.On the positive front, I've used the new overload throughout the repo to replace various custom awaiters, use of Task.Run, and places we were catching all exceptions to suppress them with awaits. Some of these just help to clean up the code; others have / enable meaningful perf improvements. For example: