1

I have an ASP.NET MVC/WebAPI application where the domain logic depends on events in certain circumstances to decouple concerns. It's becoming increasingly difficult to avoid using async methods in the event handlers, but being that this is a web application I want to avoid using async void as these are not top level events that we're dealing with. I've seen some solutions that seem overly complex for dealing with this- I'd like to keep the solution to this simple. My solution is to ditch EventHandler delegates and use a Func that returns a Task instead, e.g.:

public event EventHandler<MyEventArgs> SomethingHappened;

would be refactored to:

public event Func<object, MyEventArgs, Task> SomethingHappened;

so in my code I can do this:

if (SomethingHappened != null)
{
    await SomethingHappened.Invoke(this, new MyEventArgs());
}

We're the only one consuming these projects, so using the standard convention of EventHandler isn't absolutely necessary. While using this pattern implies knowledge that the handler is async, I'm not sure that's necessarily a bad thing as more and more libraries are dropping their synchronous API methods or simply not including them to begin with. To some extent I'm surprised this isn't supported as a first class concept within .NET with async/await becoming increasingly prevalent in many libraries that are commonly used by web applications.

This seems like an elegant solution. I've tested this in a real application with multiple subscribers to the event, with different delays for each handler, and Invoke() awaits them all. Yet, it feels like a trap. What am I missing?

joelmdev
  • 11,083
  • 10
  • 65
  • 89

1 Answers1

5

Yet, it feels like a trap. What am I missing?

This part isn't correct:

Invoke() awaits them all.

From the docs:

Invocation of a delegate instance whose invocation list contains multiple entries proceeds by invoking each of the methods in the invocation list, synchronously, in order... If the delegate invocation includes output parameters or a return value, their final value will come from the invocation of the last delegate in the list.

So, the Invoke will invoke all the handlers, but only return the Task from the last handler. The other returned tasks are ignored. Which is Very Bad (try throwing an exception from one of the ignored tasks).

Instead, you should call GetInvocationList to get the list of handlers, invoke each one, and then await them all using Task.WhenAll:

var args = new MyEventArgs();
var tasks = SomethingHappened.GetInvocationList()
    .Cast<Func<object, MyEventArgs, Task>>()
    .Select(handler => handler(this, args))
    .ToList();
await Task.WhenAll(tasks);
Stephen Cleary
  • 437,863
  • 77
  • 675
  • 810
  • 1
    Just the man whose ears I was hoping to get burning! I know this is subjective, but is this the "best" or at least a fairly acceptable way to go about this? Is there a better way? I like the above code as its fairly simple and I should be able to encapsulate it into something simple and reusable, but should I? It's starting to feel like we're overdue for some syntactic sugar to handle this as it's getting increasingly difficult to get away from asynchronous methods. – joelmdev May 06 '21 at 20:31
  • 1
    @joelmdev: It's an acceptable pattern. I usually prefer [deferrals](https://blog.stephencleary.com/2013/02/async-oop-5-events.html), but they're more complex. The task-returning delegate approach is easier (once you use `GetInvocationList` instead of just `Invoke`). – Stephen Cleary May 07 '21 at 00:36
  • 1
    Thanks as always, Stephen. I've created a generic extension method based on your code which is available [here](https://gist.github.com/joelmdev/00df42c04e08d63fbce4e517ae223d93). – joelmdev May 07 '21 at 14:30