Catching Exceptions INSIDE a delegate in a generic fashion

Question

I need help identifying a generic way of catching exceptions that are thrown from within a delegate when that delegate contains a Task.Run that is throwing an exception. Currently, when I invoke a delegate (in this case a func) that is passed into another "master" method and an exception is thrown from within a Task.Run in the delegate, the exception is not caught with a try/catch in the code surrounding the invocation. The following simplified example shows the exceptions inside the delegate being caught by the try/catch inside the delegate but being "lost" elsewhere:

public static void Main()
{
    AppDomain.CurrentDomain.UnhandledException += (s, e) => CurrentDomain_UnhandledException();
    TaskScheduler.UnobservedTaskException += (s, e) => CurrentDomain_UnhandledException();
    HandledDelegate(() => Task.Run(() =>
    {
        try
        {
            Console.WriteLine("Executed");
            throw new Exception($"Caught");
        }
        catch (Exception ex)
        {
            Console.WriteLine(ex.Message);
            throw new Exception($"Caught Again");
        }
    }

    ));
}

private static void CurrentDomain_UnhandledException()
{
    Console.WriteLine("Unhandled Exception Occurred.");
}

public static void HandledDelegate(Action a)
{
    try
    {
        a.Invoke();
    }
    catch (Exception ex)
    {
        Console.WriteLine(ex.Message);
        throw;
    }

    Thread.Sleep(10000);
}

And the following example shows the exception not being caught within the try/catch around the invoke, and bubbling up to the app domain as an unhandled exception.

public static void Main()
{
    AppDomain.CurrentDomain.UnhandledException += (s, e) => Console.WriteLine("Unhandled Exception");
    HandleDelegateEx(async () => await Task.Run(() => throw new Exception("test")));
    Thread.Sleep(1000);
}

public static void HandleDelegateEx(Action a)
{
    try
    {
        a.Invoke();
    }
    catch (Exception ex)
    {
        Console.WriteLine(ex.Message);
    }
}

This last one is most like my current code which I need to fix. The delegates often Task the code off on a background thread and don't handle any errors (expecting them to be caught by where the delegate is invoked.

This code is used throughout the app in hundreds of locations, and few, if any, catch exceptions within the delegate (as other programmers erroneously thought that exceptions thrown by the delegate would be caught). Is there any way to catch exceptions thrown from inside these delegates without massively refactoring each and every delegate to catch exceptions?

Answer 1

The issue is not connected to delegate itself. In your fiddle you don't wait for Task returned by delegate to complete. Basically the delegate will return Task and the method will continue. In your original code in question:

try
{
    // Execute the requested action.
    retval = clientOperation.Invoke(remoteObj);
}
catch(ex)
{
    // log here.
}
finally
{
    CloseChannel(remoteObj);        
}

It will jump straight to finally clause and attempt to close connection. You need to handle Func's returning tasks separately:

public void HandleDelegateEx<T>(Func<T> a)
{
    try
    {           
        a.Invoke();
    }
    catch (Exception ex)
    {
        Console.WriteLine(ex.Message);
    }
}

public async Task HandleDelegateExAsync<T>(Func<Task<T>> a)
{
    try
    {
        await a.Invoke();
    }
    catch (Exception ex)
    {
        Console.WriteLine(ex.Message);
    }
}

HandleDelegateEx<int>(() => throw new Exception("test")); // prints "test"
Func<Task<int>> func = () => Task.FromException<int>(new Exception("test async"));
HandleDelegateEx(func); // does not print anything
await HandleDelegateExAsync(func); // prints "test async"

You can get the same behavior without delegates:

Task task = null;
try
{
    task = Task.FromException(new Exception("test async"));
}
catch (Exception ex)
{
    Console.WriteLine(ex.Message); // does not go here
}
await task; // throws here

Answer 2

In order for your exceptions to be observed, you would either have to await the returned task as Guru Stron suggests (implying a massive refactoring I guess). Or somehow adding Wait() to the result of the Task.Run() call, but this will block the calling thread (I'm assuming this is the UI thread) while the background thread churns the delegate - neglecting the benefit of the task being a background thread in the first place.

So, most probably, here is the crux of the problem: you need to wait for the background Task to finish so you can observe any possible exceptions, without blocking the UI thread, and do this while modifying the least amount of code.

I'm assuming your application is either Windows Forms or WPF. For the first problem -waiting for the task to finish without blocking the UI thread-, I would turn to the old (and very frowned upon) Application.DoEvents() (or its equivalent in WPF, second answer). This is strongly discouraged these days, but I guess it is much better than ignoring exceptions that should have been handled (and keep in mind this was valid and in fact, the only way of keeping a responsive UI in the 16-bit era).

Instead of adding Wait(), to the background task, I would wait for its completion with the following loop:

while( !backgroundTask.IsCompleted ) {
   Application.DoEvents();   // keep UI responsive while background task finishes
}

Now, the second problem, we would have to somehow inject this loop right after the background task is queued in the thread pool, and as you describe, this occurs in potentially hundreds of places.

Here I would try using Harmony, a very powerful dynamic IL patching tool. This tool allows you to target a method and piggyback a prefix and/or a postfix method. The prefix method will be called automatically before the target method is called, and the postfix method will be called after the target method is called.

The patching would look something like this:

// Must be called once before the target delegates are executed
private void MyClass.Patch() {
   var original = typeof(Task).GetMethod("Run", ...);  // I'm omitting the code that would select the (Action action) overload
   var postfix = typeof(MyClass).GetMethod("WaitWithResponsiveUI", ...);
   harmony.Patch(original, postfix: new HarmonyMethod(postfix));
}


private void MyClass.WaitWithResponsiveUI(ref Action __result) {
   // __result contains the original Task returned by Task.Run()
   while( !__result.IsCompleted ) {
      Application.DoEvents();   // keep UI responsive while background task finishes
   }
   __result.Wait(); // Any exceptions produced inside the delegate should be rethrown at this moment, and they should be trappable by the code that invoked the delegate
}

Of course, this has the disadvantage that every Task.Run(Action action) called after patching would now wait until the task is done, globally, and this might not be desirable for every call.

EDIT: In defense of Application.DoEvents();

Here is a very well written answer detailing all the potential pitfalls of using Application.DoEvents(). These pitfalls are real and should be considered.

However, one aspect I strongly disagree is the assertion that the async-await mechanism is a safe way to avoid all these problems. That is simply not true.

When a UI event handler finds an await, it basically sets up a continuation that will be able to resume the event handling in the future, and immediately returns, giving the application's message pump a chance to continue running (basically what Application.DoEvents(), with the difference that the latter will return once the event queue is empty).

When the awaited operation completes, the continuation IS enqueued as another event into the Message event queue, so it won't resume execution until the message pump has handled all the events that have been posted before. My point being - While the operation is awaited, messages ARE being processed and you still need to protect your application from any reentrancy pitfalls manually. See here: Handling Reentrancy in Async Apps (C#).

Another pitfall exclusive to async-await UI code is that some components (DataGridView comes to mind) pass some of their event arguments to their handlers in objects that implement IDisposable. The component raises the event, and disposes the arguments as soon as the event handler returns. In async code, this return to the caller will happen when the first await in the handler code is reached. This has the effect that when the awaited code resumes, accessing the event-arguments object throws an ObjectDisposedException, as it has already been disposed by the raising component itself!

The "false multithreading" aspect is already being handled here, as the work is truly being offloaded to the thread pool.

So, in my opinion, the only real problem to address is the potential stack-overflow problem, that could be addressed by reentrancy avoidance measures that have to be accounted for in the first place, whether you use async/await or DoEvents().