From sync to async scenario

Question

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?

Answer 1

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.

Answer 2

Your colleague is correct, the posted example code will most likely not work as intended.

I would suggest making ShutdownRequestedEventArgs contain something like a List<Task<bool>>, so the caller can await all the task to determine if any of them wants to cancel before shutting down. This might also include a timeout, so that some hanged task does not block the process forever. This moves the problem from the handler of the event to the caller, that hopefully is in a better position to deal with the problem.

Another possibility could be to wait for the task synchronously:

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

But this may be dangerous in a UI program. If the ShutdownRequested is requested on the UI thread, and CanShutdown needs to execute any code on the UI thread, then the program will deadlock. And this is fairly likely if CanShutdown is not written specifically to avoid this. See configureAwait for more details .

You might also just revert to the synchronous solution. An event like ShutdownRequested does not sound like it is a good fit for an asynchronous solution. I would have expected that such an event would require a immediate response. But I do not know the background of your application or why the method was changed in the first place, so there might very well be good reasons for it.