Skip to content
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

Merged
merged 3 commits into from
Jan 29, 2021
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions sdk/core/Azure.Core/api/Azure.Core.net461.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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)) { }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why no object sender?

Copy link
Member Author

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 the SearchIndexingBufferedSender.

public System.Threading.CancellationToken CancellationToken { get { throw null; } }
public bool RunSynchronously { get { throw null; } }
}
}
namespace Azure.Core
{
Expand Down Expand Up @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no TClient?

Copy link
Member Author

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 the SearchIndexingBufferedSender.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should Azure.SyncAsyncEventArgs have TSender Sender read only property? Or is it missing to leave a room for cases where sender should not be exposed (if such exist).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you'll always need to derive from SyncAsyncEventArgs if you're doing anything interesting enough to need a client, at which point you might as well add it yourself with a better name (like public QueueClient Queue { get; } for your use case). I did use Sender for the Search examples, but that's only because the relevant type is actually a SearchIndexingBufferedSender rather than a SearchClient.

I think the only reason it'd be worth adding a generic TSender is if we think we'll have a lot of events with no extra data. I'd be surprised if we end up with any of those.

public abstract partial class TokenCredential
{
protected TokenCredential() { }
Expand Down
7 changes: 7 additions & 0 deletions sdk/core/Azure.Core/api/Azure.Core.net5.0.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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
{
Expand Down Expand Up @@ -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;
public abstract partial class TokenCredential
{
protected TokenCredential() { }
Expand Down
7 changes: 7 additions & 0 deletions sdk/core/Azure.Core/api/Azure.Core.netstandard2.0.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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
{
Expand Down Expand Up @@ -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;
public abstract partial class TokenCredential
{
protected TokenCredential() { }
Expand Down
191 changes: 191 additions & 0 deletions sdk/core/Azure.Core/samples/Events.md
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
Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are no docs (yet) and I don't think the source adds any value compared to what's here (i.e., Handler and Args).

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
Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Member Author

Choose a reason for hiding this comment

The 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Link to section header?

Copy link
Member Author

Choose a reason for hiding this comment

The 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
executed sequentially to avoid introducing any unintended parallelism. The
executed sequentially to avoid introducing any unintended parallelism to calling code. The

Copy link
Member Author

Choose a reason for hiding this comment

The 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,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Link here to the section that describes how exceptions work here?

Copy link
Member Author

Choose a reason for hiding this comment

The 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) =>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need an example with unsubscribing from event.

Copy link
Member Author

Choose a reason for hiding this comment

The 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.
2 changes: 2 additions & 0 deletions sdk/core/Azure.Core/samples/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,4 +14,6 @@ description: Samples for the Azure.Core client library
- [Response](https://github.com/Azure/azure-sdk-for-net/blob/master/sdk/core/Azure.Core/samples/Response.md)
- [Pipeline](https://github.com/Azure/azure-sdk-for-net/blob/master/sdk/core/Azure.Core/samples/Pipeline.md)
- [Long Running Operations](https://github.com/Azure/azure-sdk-for-net/blob/master/sdk/core/Azure.Core/samples/LongRunningOperations.md)
- [Events](https://github.com/Azure/azure-sdk-for-net/blob/master/sdk/core/Azure.Core/samples/Events.md)
- [Diagnostics](https://github.com/Azure/azure-sdk-for-net/blob/master/sdk/core/Azure.Core/samples/Diagnostics.md)
- [Mocking](https://github.com/Azure/azure-sdk-for-net/blob/master/sdk/core/Azure.Core/samples/Mocking.md)
123 changes: 123 additions & 0 deletions sdk/core/Azure.Core/src/Shared/SyncAsyncEventHandlerExtensions.cs
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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should take in a CancellationToken and check it between iterations so we don't continue executing handlers.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch. I debated this a lot. We do take one via SyncAsyncEventArgs.CancellationToken. Calling ThrowIfCancellationRequested is kind of expensive though so I didn't want to do it unnecessarily between handlers. I figured if anyone does anything particularly slow, that operation would check. (The TestHandler does this to verify it works correctly.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. Might want to suggest that in the XML docs.

Copy link
Member Author

Choose a reason for hiding this comment

The 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:

Suggested change
// (We're not bother to check our tests because
// (We'll not bother to check our tests because

Copy link
Member Author

Choose a reason for hiding this comment

The 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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No special handling for OperationCanceledException?

Copy link
Member Author

Choose a reason for hiding this comment

The 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;
}
}
}
}
Loading