-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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 SyncAsyncEventHandler #18170
Add SyncAsyncEventHandler #18170
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -181,6 +181,12 @@ protected Response() { } | |
public static implicit operator T (Azure.Response<T> response) { throw null; } | ||
public override string ToString() { throw null; } | ||
} | ||
public partial class SyncAsyncEventArgs : System.EventArgs | ||
{ | ||
public SyncAsyncEventArgs(bool runSynchronously, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { } | ||
public System.Threading.CancellationToken CancellationToken { get { throw null; } } | ||
public bool RunSynchronously { get { throw null; } } | ||
} | ||
} | ||
namespace Azure.Core | ||
{ | ||
|
@@ -405,6 +411,7 @@ internal RetryOptions() { } | |
public Azure.Core.RetryMode Mode { get { throw null; } set { } } | ||
public System.TimeSpan NetworkTimeout { get { throw null; } set { } } | ||
} | ||
public delegate System.Threading.Tasks.Task SyncAsyncEventHandler<T>(T e) where T : Azure.SyncAsyncEventArgs; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no TClient? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @KrzysztofCwalina wants to put that in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you'll always need to derive from I think the only reason it'd be worth adding a generic |
||
public abstract partial class TokenCredential | ||
{ | ||
protected TokenCredential() { } | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,191 @@ | ||||||
# Azure.Core Event samples | ||||||
|
||||||
**NOTE:** Samples in this file only apply to packages following the | ||||||
[Azure SDK Design Guidelines](https://azure.github.io/azure-sdk/dotnet_introduction.html). | ||||||
The names of these packages usually start with `Azure`. | ||||||
|
||||||
Most Azure client libraries for .NET offer both synchronous and asynchronous | ||||||
methods for calling Azure services. You can distinguish the asynchronous | ||||||
methods by their `Async` suffix. For example, `BlobClient.Download` and | ||||||
`BlobClient.DownloadAsync` make the same underlying REST call and only differ in | ||||||
whether they block. We recommend using our async methods for new applications, | ||||||
but there are perfectly valid cases for using sync methods as well. These dual | ||||||
method invocation semantics address the needs of our customers, but require a | ||||||
little extra care when writing event handlers. | ||||||
|
||||||
The `SyncAsyncEventHandler` is a delegate used by events in Azure client | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For event handler and event args ... link to the source or docs so folks can learn more? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||||||
libraries to represent an event handler that can be invoked from either sync or | ||||||
async code paths. It takes event arguments deriving from `SyncAsyncEventArgs` | ||||||
that contain important information for writing your event handler. | ||||||
|
||||||
- `SyncAsyncEventArgs.CancellationToken` is a cancellation token related to the | ||||||
original operation that raised the event. It's important for your handler to | ||||||
pass this token along to any asynchronous or long-running synchronous | ||||||
operations that take a token so cancellation (via something like | ||||||
`new CancellationTokenSource(TimeSpan.FromSeconds(10)).Token`, for example) | ||||||
will correctly propagate. | ||||||
|
||||||
- There is a `SyncAsyncEventArgs.RunSynchronously` flag indicating whether your | ||||||
handler was invoked synchronously or asynchronously. In general, | ||||||
|
||||||
- If you're calling sync methods on your client, you should use sync methods | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not clear to me if the reader you're addressing here is the library user or library developer. Would it be possible to clarify? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The whole document is for users of the library. |
||||||
to implement your event handler. You can return `Task.CompletedTask`. | ||||||
- If you're calling async methods on your client, you should use async | ||||||
methods where possible to implement your event handler. | ||||||
- If you're not in control of how the client will be used or want to write | ||||||
safer code, you should check the `RunSynchronously` property and call | ||||||
either sync or async methods as directed. | ||||||
|
||||||
There are code examples of all three situations below to compare. Please also | ||||||
see the note at the very end discussing the dangers of sync-over-async to | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Link to section header? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I haven't seen that in any other Azure.Core samples and it feels a little short for links between sections. |
||||||
understand the risks of not using the `RunSynchronously` flag. | ||||||
|
||||||
- Most events will customize the event data by deriving from `SyncAsyncEventArgs` | ||||||
and including details about what triggered the event or providing options to | ||||||
react. Many times this will include a reference to the client that raised the | ||||||
event in case you need it for additional processing. | ||||||
|
||||||
When an event using `SyncAsyncEventHandler` is raised, the handlers will be | ||||||
executed sequentially to avoid introducing any unintended parallelism. The | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think "calling code" muddies it a bit since we're avoiding unintentional parallelism in the handlers. |
||||||
event handlers will finish before returning control to the code path raising the | ||||||
event. This means blocking for events raised synchronously and waiting for the | ||||||
returned `Task` to complete for events raised asynchronously. Any exceptions | ||||||
thrown from a handler will be wrapped in a single `AggregateException`. Finally, | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Link here to the section that describes how exceptions work here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I haven't seen that in any other Azure.Core samples and it feels a little short for links between sections. |
||||||
a [distributed tracing span](https://github.com/Azure/azure-sdk-for-net/blob/master/sdk/core/Azure.Core/samples/Diagnostics.md#distributed-tracing) | ||||||
is wrapped around your handlers using the event name so you can see how long | ||||||
your handlers took to run, whether they made other calls to Azure services, and | ||||||
details about any exceptions that were thrown. | ||||||
|
||||||
The rest of the code samples are using a fictitious `AlarmClient` to demonstrate | ||||||
how to handle `SyncAsyncEventHandler` events. There are `Snooze` and | ||||||
`SnoozeAsync` methods that both raise a `Ring` event. | ||||||
|
||||||
## Adding a synchronous event handler | ||||||
|
||||||
If you're using the synchronous, blocking methods of a client (i.e., methods | ||||||
without an `Async` suffix), they will raise events that require handlers to | ||||||
execute synchronously as well. Even though the signature of your handler | ||||||
returns a `Task`, you should write regular sync code that blocks and return | ||||||
`Task.CompletedTask` when finished. | ||||||
|
||||||
```C# Snippet:Azure_Core_Samples_EventSamples_SyncHandler | ||||||
var client = new AlarmClient(); | ||||||
client.Ring += (SyncAsyncEventArgs e) => | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Need an example with unsubscribing from event. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think there's a good reason to. There's nothing special about our model that changes unsubscribing and it's not a thing we'd expect most customers to do. |
||||||
{ | ||||||
Console.WriteLine("Wake up!"); | ||||||
return Task.CompletedTask; | ||||||
}; | ||||||
|
||||||
client.Snooze(); | ||||||
``` | ||||||
|
||||||
If you need to call an async method from a synchronous event handler, you have | ||||||
two options: | ||||||
|
||||||
- You can use [`Task.Run`](https://docs.microsoft.com/dotnet/api/system.threading.tasks.task.run) | ||||||
to queue a task for execution on the ThreadPool without waiting on it to | ||||||
complete. This "fire and forget" approach may not run before your handler | ||||||
finishes executing. Be sure to understand | ||||||
[exception handling in the Task Parallel Library](https://docs.microsoft.com/dotnet/standard/parallel-programming/exception-handling-task-parallel-library) | ||||||
to avoid unhandled exceptions tearing down your process. | ||||||
- If you absolutely need the async method to execute before returning from your | ||||||
handler, you can call `myAsyncTask.GetAwaiter().GetResult()`. Please be aware | ||||||
this may cause ThreadPool starvation. See the sync-over-async note below for | ||||||
more details. | ||||||
|
||||||
## Adding an asynchronous event handler | ||||||
|
||||||
If you're using the asynchronous, non-blocking methods of a client (i.e., | ||||||
methods with an `Async` suffix), they will raise events that expect handlers to | ||||||
execute asynchronously. | ||||||
|
||||||
```C# Snippet:Azure_Core_Samples_EventSamples_AsyncHandler | ||||||
var client = new AlarmClient(); | ||||||
client.Ring += async (SyncAsyncEventArgs e) => | ||||||
{ | ||||||
await Console.Out.WriteLineAsync("Wake up!"); | ||||||
}; | ||||||
|
||||||
await client.SnoozeAsync(); | ||||||
``` | ||||||
|
||||||
## Handlers that can be called sync or async | ||||||
|
||||||
The same event can be raised from both synchronous and asynchronous code paths | ||||||
depending on whether you're calling sync or async methods on a client. If you | ||||||
write an async handler but raise it from a sync method, the handler will be | ||||||
doing sync-over-async and may cause ThreadPool starvation. See the note at the | ||||||
bottom for more details. | ||||||
|
||||||
You should use the `SyncAsyncEventArgs.RunSynchronously` property to check how | ||||||
the event is being raised and implement your handler accordingly. Here's an | ||||||
example handler that's safe to invoke from both sync and async code paths. | ||||||
|
||||||
```C# Snippet:Azure_Core_Samples_EventSamples_CombinedHandler | ||||||
var client = new AlarmClient(); | ||||||
client.Ring += async (SyncAsyncEventArgs e) => | ||||||
{ | ||||||
if (e.RunSynchronously) | ||||||
{ | ||||||
Console.WriteLine("Wake up!"); | ||||||
} | ||||||
else | ||||||
{ | ||||||
await Console.Out.WriteLineAsync("Wake up!"); | ||||||
} | ||||||
}; | ||||||
|
||||||
client.Snooze(); // sync call that blocks | ||||||
await client.SnoozeAsync(); // async call that doesn't block | ||||||
``` | ||||||
|
||||||
## Handling exceptions | ||||||
|
||||||
Any exceptions thrown by an event handler will be wrapped in a single | ||||||
[`AggregateException`](https://docs.microsoft.com/dotnet/api/system.aggregateexception) and thrown from the code that raised the event. You can check the | ||||||
[`AggregateException.InnerExceptions`](https://docs.microsoft.com/dotnet/api/system.aggregateexception.innerexceptions) | ||||||
property to see the original exceptions thrown by your event handlers. | ||||||
`AggregateException` also provides | ||||||
[a number of helpful methods](https://docs.microsoft.com/archive/msdn-magazine/2009/brownfield/aggregating-exceptions) | ||||||
like `Flatten` and `Handle` to make complex failures easier to work with. | ||||||
|
||||||
```C# Snippet:Azure_Core_Samples_EventSamples_Exceptions | ||||||
var client = new AlarmClient(); | ||||||
client.Ring += (SyncAsyncEventArgs e) => | ||||||
throw new InvalidOperationException("Alarm unplugged."); | ||||||
|
||||||
try | ||||||
{ | ||||||
client.Snooze(); | ||||||
} | ||||||
catch (AggregateException ex) | ||||||
{ | ||||||
ex.Handle(e => e is InvalidOperationException); | ||||||
Console.WriteLine("Please switch to your backup alarm."); | ||||||
} | ||||||
``` | ||||||
|
||||||
## Sync-over-async | ||||||
|
||||||
Executing asynchronous code from a sync code path is commonly referred to as | ||||||
sync-over-async because you're getting sync behavior but still invoking all the | ||||||
async machinery. See | ||||||
[Diagnosing .NET Core ThreadPool Starvation with PerfView](https://docs.microsoft.com/archive/blogs/vancem/diagnosing-net-core-threadpool-starvation-with-perfview-why-my-service-is-not-saturating-all-cores-or-seems-to-stall) | ||||||
for a detailed explanation of how that can cause serious performance problems. | ||||||
We recommend you use the `SyncAsyncEventArgs.RunSynchronously` flag to avoid | ||||||
ThreadPool starvation. | ||||||
|
||||||
But what about executing synchronous code on an async code path like the "Adding | ||||||
a synchronous event handler" code sample above? This is perfectly okay. Behind | ||||||
the scenes, we're effectively doing something like: | ||||||
|
||||||
```C# | ||||||
var task = InvokeHandler(); | ||||||
if (!task.IsCompleted) | ||||||
{ | ||||||
task.Wait(); | ||||||
} | ||||||
``` | ||||||
|
||||||
Writing sync code in your handler will block before returning a completed `Task` | ||||||
so there's no need to involve the ThreadPool to run your handler. |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,123 @@ | ||||||
// Copyright (c) Microsoft Corporation. All rights reserved. | ||||||
// Licensed under the MIT License. | ||||||
|
||||||
using System; | ||||||
using System.Collections.Generic; | ||||||
using System.Threading.Tasks; | ||||||
using Azure.Core.Pipeline; | ||||||
|
||||||
namespace Azure.Core | ||||||
{ | ||||||
/// <summary> | ||||||
/// Extensions for raising <see cref="SyncAsyncEventHandler{T}"/> | ||||||
/// events. | ||||||
/// </summary> | ||||||
internal static class SyncAsyncEventHandlerExtensions | ||||||
{ | ||||||
/// <summary> | ||||||
/// Raise an <see cref="Azure.Core.SyncAsyncEventHandler{T}"/> | ||||||
/// event by executing each of the handlers sequentially (to avoid | ||||||
/// introducing accidental parallelism in customer code) and collecting | ||||||
/// any exceptions. | ||||||
/// </summary> | ||||||
/// <typeparam name="T">Type of the event arguments.</typeparam> | ||||||
/// <param name="eventHandler">The event's delegate.</param> | ||||||
/// <param name="e"> | ||||||
/// An <see cref="SyncAsyncEventArgs"/> instance that contains the | ||||||
/// event data. | ||||||
/// </param> | ||||||
/// <param name="declaringTypeName"> | ||||||
/// The name of the type declaring the event to construct a helpful | ||||||
/// exception message and distributed tracing span. | ||||||
/// </param> | ||||||
/// <param name="eventName"> | ||||||
/// The name of the event to construct a helpful exception message and | ||||||
/// distributed tracing span. | ||||||
/// </param> | ||||||
/// <param name="clientDiagnostics"> | ||||||
/// Client diagnostics to wrap all the handlers in a new distributed | ||||||
/// tracing span. | ||||||
/// </param> | ||||||
/// <returns> | ||||||
/// A task that represents running all of the event's handlers. | ||||||
/// </returns> | ||||||
/// <exception cref="AggregateException"> | ||||||
tg-msft marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
/// An exception was thrown during the execution of at least one of the | ||||||
/// event's handlers. | ||||||
/// </exception> | ||||||
/// <exception cref="ArgumentNullException"> | ||||||
/// Thrown when <paramref name="e"/>, <paramref name="declaringTypeName"/>, | ||||||
/// <paramref name="eventName"/>, or <paramref name="clientDiagnostics"/> | ||||||
/// are null. | ||||||
/// </exception> | ||||||
/// <exception cref="ArgumentException"> | ||||||
/// Thrown when <paramref name="declaringTypeName"/> or | ||||||
/// <paramref name="eventName"/> are empty. | ||||||
/// </exception> | ||||||
public static async Task RaiseAsync<T>( | ||||||
this SyncAsyncEventHandler<T> eventHandler, | ||||||
T e, | ||||||
string declaringTypeName, | ||||||
string eventName, | ||||||
ClientDiagnostics clientDiagnostics) | ||||||
where T : SyncAsyncEventArgs | ||||||
{ | ||||||
Argument.AssertNotNull(e, nameof(e)); | ||||||
Argument.AssertNotNullOrEmpty(declaringTypeName, nameof(declaringTypeName)); | ||||||
Argument.AssertNotNullOrEmpty(eventName, nameof(eventName)); | ||||||
Argument.AssertNotNull(clientDiagnostics, nameof(clientDiagnostics)); | ||||||
|
||||||
// Get the invocation list, but return early if there's no work | ||||||
if (eventHandler == null) { return; } | ||||||
Delegate[] handlers = eventHandler.GetInvocationList(); | ||||||
if (handlers == null || handlers.Length == 0) { return; } | ||||||
|
||||||
// Wrap handler invocation in a distributed tracing span so it's | ||||||
// easy for customers to track and measure | ||||||
string eventFullName = declaringTypeName + "." + eventName; | ||||||
using DiagnosticScope scope = clientDiagnostics.CreateScope(eventFullName); | ||||||
scope.Start(); | ||||||
try | ||||||
{ | ||||||
// Collect any exceptions raised by handlers | ||||||
List<Exception> failures = null; | ||||||
|
||||||
// Raise the handlers sequentially so we don't introduce any | ||||||
// unintentional parallelism in customer code | ||||||
foreach (Delegate handler in handlers) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should take in a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice catch. I debated this a lot. We do take one via There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makes sense. Might want to suggest that in the XML docs. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good idea - fixed. |
||||||
{ | ||||||
SyncAsyncEventHandler<T> azureHandler = (SyncAsyncEventHandler<T>)handler; | ||||||
try | ||||||
{ | ||||||
Task runHandlerTask = azureHandler(e); | ||||||
// We can consider logging something when e.RunSynchronously | ||||||
// is true, but runHandlerTask.IsComplete is false. | ||||||
// (We're not bother to check our tests because | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit:
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed |
||||||
// EnsureCompleted on the code path that raised the | ||||||
// event will catch it for us.) | ||||||
await runHandlerTask.ConfigureAwait(false); | ||||||
} | ||||||
catch (Exception ex) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No special handling for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, see the discussion around Heath's comment. |
||||||
{ | ||||||
failures ??= new List<Exception>(); | ||||||
failures.Add(ex); | ||||||
} | ||||||
} | ||||||
|
||||||
// Wrap any exceptions in an AggregateException | ||||||
if (failures?.Count > 0) | ||||||
{ | ||||||
// Include the event name in the exception for easier debugging | ||||||
throw new AggregateException( | ||||||
"Unhandled exception(s) thrown when raising the " + eventFullName + " event.", | ||||||
failures); | ||||||
} | ||||||
} | ||||||
catch (Exception ex) | ||||||
{ | ||||||
scope.Failed(ex); | ||||||
throw; | ||||||
} | ||||||
} | ||||||
} | ||||||
} |
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 no
object sender
?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.
@KrzysztofCwalina wants to put that in the
EventArgs
. If you look at the Search APIs in particular you'll see they have an instance of theSearchIndexingBufferedSender
.