From sync to async scenario

I have this event handler:

private void OnShutdownRequested(object? sender, ShutdownRequestedEventArgs e)
{
   var canShutdown = lifetime.CanShutdown();
   e.Cancel = !canShutdown;
}

Now, due to design decisions the CanShutdown method has changed from bool to Task<bool>

Task<bool> CanShutDown()
{
   //...
}

So, I need to modify the event handler like this:

private async void OnShutdownRequested(object? sender, ShutdownRequestedEventArgs e)
{
   var canShutdown = await lifetime.CanShutdown();
   e.Cancel = !canShutdown;
}

I've read many times that async void methods have many problems, like unhandled exceptions being thrown directly inside the SynchronizationContext. But one of the valid usages for them are event handlers. This is an event handler. Isn't it?

BUT, I wonder if that code is free of undesired consequences after the "migration".

My concern is that the handler modifies the value of e.Cancel.

A colleague has told me that this will happen:

After await, the caller to that method isn't awaiting. It assumes synchronous execution, so it immediately reads e.Cancel, and that hasn't been set yet. That is a problem inside event handler: You realize as soon as the await keyword is hit, the code that called ShutdownRequested.Invoke() immediately returns. The value it will read might not be up-to-date.

I'm afraid my colleague has his point. So, it seems this approach is broken. But I still don't see how to fix that.

How to deal with the EventArgs being shared by sync and async code?

Solution

I've read many times that async void methods have many problems, like unhandled exceptions being thrown directly inside the SynchronizationContext. But one of the valid usages for them are event handlers.

Yes. In fact, throwing exceptions on the SynchronizationContext is actually deliberate behavior specifically to emulate the behavior of event handlers.

One of the primary problems with async void method is that it's not easy to determine when the async void method has completed. Which is exactly the problem your colleague is pointing out.

This is an event handler. Isn't it?

Sort of.

Take a step back and consider the design patterns being used. 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 kind of "shutdown" notification is not just a notification, though. It also has a return value. Generally, this is the Strategy pattern. The Strategy pattern is not a good fit for events in OOP. However, many times the Strategy pattern is (mis-)implemented with an event; this is a common design mistake in OOP languages.

So, is it an event handler? Technically, yes. Should it be an event handler? Probably not.

In the synchronous world, implementing the Strategy pattern with events is often "close enough". Sometimes there's some confusion about how the return value should be handled in case there are multiple event subscribers, but in general this kind of design mistake goes unnoticed. Until async comes along, and suddenly the design mistake of using events for the Strategy pattern becomes more apparent.

But I still don't see how to fix that.

I describe a few possibilities on my blog.

If you control OnShutdownRequested and ShutdownRequestedEventArgs, then you can use deferrals. It's a bit complex to set up, but it allows both synchronous and asynchronous handlers (as long as the handlers use a deferral), and the code that raises the event can (asynchronously) wait for all handlers to complete before retrieving the results.

If shutdown is the only event you have to worry about, then one common trick is to always set Cancel to true, do the asynchronous work, and then if the shutdown is permitted, explicitly do a shutdown at the end of that asynchronous work.