Given the following code:
public delegate Task AsyncEventHandler<in TEventArgs>(object sender, TEventArgs eventArgs);
public static Task InvokeAsync(this AsyncEventHandler eventHandler, object sender, EventArgs eventArgs)
{
if (eventHandler == null) return Task.CompletedTask;
var delegates = eventHandler.GetInvocationList().Cast<AsyncEventHandler>();
var tasks = delegates.Select(it => it.Invoke(sender, eventArgs));
return Task.WhenAll(tasks);
}
I have a test function whereby the faulty handlers should throw exceptions, and the workinghanlder should run - currently only the first FaultyHandler1 is called and no others event handlers.
private class NonGenericNotifier
{
public event AsyncEventHandler SomethingHappened;
public Task OnSomethingHappening() => SomethingHappened.InvokeAsync(this, EventArgs.Empty);
}
public async Task Multiple_Exceptions_That_Occur_During_Event_Handling_Should_Be_Propagated()
{
var isHandler1Called = false;
var isHandler2Called = false;
var isWorkingHandlerCalled = false;
var notifier = new NonGenericNotifier();
Task FaultyHandler1(object sender, EventArgs eventArgs)
{
isHandler1Called = true;
throw new InvalidOperationException();
}
Task FaultyHandler2(object sender, EventArgs eventArgs)
{
isHandler2Called = true;
throw new InvalidOperationException();
}
Task WorkingHandler(object sender, EventArgs eventArgs)
{
isWorkingHandlerCalled = true;
return Task.CompletedTask;
}
notifier.SomethingHappened += FaultyHandler1;
notifier.SomethingHappened += FaultyHandler2;
notifier.SomethingHappened += WorkingHandler;
await Should.ThrowAsync<InvalidOperationException>(async () => await notifier.OnSomethingHappening());
isHandler1Called.ShouldBe(true);
isHandler2Called.ShouldBe(true);
isWorkingHandlerCalled.ShouldBe(true);
}
Assuming a single exception can be thrown I beleive this should be an AggregateException containing an exception for each Task, and most importantly the InvokeAsync method above should bail on the first exception encountered.
I have started to create a List<Exception> within the InvokeAsync extension method, and wrap each it => it.Invoke(sender, eventArgs) with a try/catch construct and within the catch add the exception to the exception list.
However I am lost on how to collate this list of exceptions and then send on as an AggregateException.
UPDATE (FIX?)
Thanks to Artur for pointing me in the right direction. I changed the InvokeAsync extension method to the below, and it works - no longer halting on the first task. We have gone from var tasks = delegates.Select(it => it.Invoke(sender, eventArgs)); to the below using the code here:
public static Task InvokeAsync(this AsyncEventHandler eventHandler, object sender, EventArgs eventArgs)
{
if (eventHandler == null) return Task.CompletedTask;
var delegates = eventHandler.GetInvocationList().Cast<AsyncEventHandler>();
var tasks = delegates.Select(async it => await it.Invoke(sender, eventArgs));
return Task.WhenAll(tasks).WithAggregatedExceptions();
}
static Task WithAggregatedExceptions(this Task task)
{
// using AggregateException.Flatten as a bonus
return task.ContinueWith(
continuationFunction: priorTask =>
priorTask.IsFaulted &&
priorTask.Exception is AggregateException ex && (ex.InnerExceptions.Count > 1 || ex.InnerException is AggregateException) ? Task.FromException(ex.Flatten()) : priorTask,
cancellationToken: CancellationToken.None,
TaskContinuationOptions.ExecuteSynchronously,
scheduler: TaskScheduler.Default).Unwrap();
}
My issue is subscribers of this event, writing synchronous handlers over which I have no control - this would stop the other event handlers (sync and async) running that are attached to the same event.
I also appreciate this is the designed function of Task.WhenAll if you were mixing async and non-async handlers... if there is one reason to not write synchronous code in an async function without await Task.Yield() this is it.
Question
Can we say that wrapping the delegates.Select(async it => await it.Invoke(sender, eventArgs) with async/await allows synchronous method to run, and at worst(?) wrap twice an async method (which is the same as nesting async/await function calls) so is actually a non-issue?
Are there any side effects that have been introduced?
With the bounty looking for authorative guidance on how this would be implemented, one answer (much appreciated for contributing to the discussion) says to avoid async events, yet in other places like the discord c# client they have embraced async events (with timeout wrappers etc).
Task.WhenAll will invoke all the handlers when it reifies its parameter. It will invoke them one at a time, and then will asynchronously wait for all the tasks to complete.
The reason you were seeing the halting on the first exception was because the exception was thrown during reification. It's normal for asynchronous (Task-returning) functions to place any exceptions on the returned task. It's abnormal for asynchronous functions to throw exceptions directly.
So, this is the problematic code:
Task FaultyHandler1(object sender, EventArgs eventArgs)
{
isHandler1Called = true;
throw new InvalidOperationException();
}
One of these would be correct:
async Task FaultyHandler1(object sender, EventArgs eventArgs)
{
isHandler1Called = true;
throw new InvalidOperationException();
}
Task FaultyHandler1(object sender, EventArgs eventArgs)
{
isHandler1Called = true;
return Task.FromException(new InvalidOperationException());
}
You're seeing the odd behavior because the asynchronous handler is misbehaving (by throwing a synchronous exception).
Now, if you want to allow misbehaving asynchronous handlers, you can do that, either with an explicit try/catch or the extra async/await:
var tasks = delegates.Select(it => try { return it.Invoke(sender, eventArgs); } catch (Exception ex) { return Task.FromException(ex); });
// async/await is necessary to handle misbehaving asynchronous handlers that throw synchronous exceptions
var tasks = delegates.Select(async it => await it.Invoke(sender, eventArgs));
If you do keep the async/await approach, please do comment it, because coding constructs like that are often assumed to be spurious and may be removed by a future maintainer.
The WithAggregatedExceptions looks fine as-is, but it can be simplified if you want:
static async Task WithAggregatedExceptions(this Task task)
{
try { await task; }
catch { throw task.Exception; }
}
My issue is subscribers of this event, writing synchronous handlers over which I have no control - this would stop the other event handlers (sync and async) running that are attached to the same event.
Well, yes. Task.WhenAll reifies its collection of tasks synchronously, which invokes all the handlers one at a time.
If you want to allow synchronous handlers as well as asynchronous ones all at the same time, you can wrap the invocations in a Task.Run:
var tasks = delegates.Select(it => Task.Run(() => it.Invoke(sender, eventArgs)));
Can we say that wrapping the delegates.Select(async it => await it.Invoke(sender, eventArgs) with async/await allows synchronous method to run, and at worst(?) wrap twice an async method (which is the same as nesting async/await function calls) so is actually a non-issue?
The extra async/await for asynchronous handlers is a non-issue; it's very slightly less efficient, and appears unnecessary, so I'd say it's in danger of being removed (unless commented). It doesn't "allow synchronous methods to run"; instead, it corrects the misbehaving methods that throw exceptions directly instead of placing them on the returned Task as expected.
Are there any side effects that have been introduced?
Not really. If you do use the Task.Run approach, then all the handlers are invoked on thread pool threads and may run concurrently, which may be surprising.
one answer (much appreciated for contributing to the discussion) says to avoid async events, yet in other places like the discord c# client they have embraced async events (with timeout wrappers etc).
I am 100% in agreement with that answer.
Here's how I think about it:
The Observer pattern is a way to notify observers of state changes. The Observer pattern is a clear fit for "events" in OOP: any number of observers may subscribe to state change notifications. This is what C# events were patterned after: notifying subscribers of things. There's no mechanism for information to "flow back". While the language allows C# events with return values, it's not natural by any means. The same limitation happens with exceptions (which can be considered a kind of return): the standard handler?.Invoke pattern starts to break (stopping invocations at the first exception, etc).
As soon as you have information "flowing back" (including needing to handle exceptions, or needing to await all the handlers to complete), you're no longer in the Observer pattern, and you are no longer in the happy path of C# events.
Generally, I find most "events" of this type are usually related to a Strategy or Visitor pattern, rather than Observer. Neither Strategy nor Visitor are good fits for C# events, although they are often (sadly) implemented that way. I consider that a common design mistake (for all OOP languages).