1
votes

I'm writing a ASP.NET MVC site in .NET Core. I'm trying to encapsulate some common exception handling. In a Base class I have this method.

public abstract class BaseController<TController> : Controller where TController : Controller
{
    protected IActionResult ExceptionHandledOperation(Func<IActionResult> operation, Func<IActionResult> handleException)
    {
        try
        {
            return operation.Invoke();
        }
        catch (Exception exception)
        {
            Logger.LogError($"Operation {Request.Path} Exception", exception);

            return handleException.Invoke();
        }
    }
}

From a controller that inherits from this base class, I utilize this method like so:

[Route("api/[controller]")]
public class MyController : BaseController<MyController>
{
    [HttpGet]
    public IActionResult Get()
    {
        return ExceptionHandledOperation(() => Ok(_someService.GetAsync().Result),
                                         NotFound);
    }
}

Assume the _someService.GetAsync() method is this:

public class SomeService
{
    public async Task<PreconfigurationData> GetAsync()
    {
        // http request removed for brevity

        if (!response.IsSuccessStatusCode)
            throw new Exception("SomeService Exception");
    }
}

This worked fine and would catch my exception in the base class method and return the NotFound result.

However, I wanted to avoid calling .Result from the SomeService.GetAsync method. Everywhere I read it says not to do that as it can deadlock.

So I modified my base controller to this:

public abstract class BaseController<TController> : Controller where TController : Controller
{
    protected async Task<IActionResult> ExceptionHandledOperationAsync(Func<IActionResult> operation, Func<IActionResult> handleException)
    {
        try
        {
            return await Task.Run(() => operation.Invoke());
        }
        catch (Exception exception)
        {
            Logger.LogError($"Operation {Request.Path} Exception", exception);

            return await Task.Run(() => handleException.Invoke());
        }
    }
}

And MyController like this:

[Route("api/[controller]")]
public class MyController : BaseController<MyController>
{
    [HttpGet]
    public async Task<IActionResult> Get()
    {
        return await ExceptionHandledOperationAsync(() => Ok(_someService.GetAsync()),
                                                    NotFound);
    }
}

However, my exception thrown from my SomeService.GetAsync method is never caught and I never get the NotFound response I intend to send when handling the exception.

Everywhere I read it says you just need to await the task in the try and then any exceptions will be caught, but mine never does.


Solved

I was finally able to get this resolved. Thanks to Tseng for the help.

3
Don't call Task.Run in ASP.NET Core application. You won't have a single benefit from it and may even reduce your performance and you may mess up with the ThreadPool management. Use await/async only operation which really run async (File IO, Network, Database access, etc.)Tseng
msdn.microsoft.com/en-us/magazine/dn802603.aspx its about ASP.NET but still applies to ASP.NET CoreTseng
@Tseng I don't understand how to make operation.Invoke() asynchronous without using Task.Run. There is no operation.InvokeAsync(). I tried using a TaskFactory.FromAsync(operation.BeginInvoke, operation.EndInvoke, null), but that threw an exception.Ristogod
Unless the operation you call is already async and real async operation (reading file, doing a network request or database request etc). just run it synchronously. There is no benefit of running sync operation async in ASP.NET Core, it may even hurt your performance. This is different than Desktop development where you use it to run stuff in background and not lock UI threadTseng
Otherwise to run async code within the callback, turn it to return Task: Func<Task<IActionResult>, then you can do something like then do something like: () => await _someService.GetAsync();. and return await handleException.Invoke();Tseng

3 Answers

1
votes

There is a logic error in your code. You call return await Task.Run(() => handleException.Invoke()); but inside the function delegate you run async code without awaiting it (here: await ExceptionHandledOperationAsync(() => Ok(_someService.GetAsync()), NotFound).

So inside your try/catch block, the method is executed and immediately returns, before the async call is finished.

Like pointed in my commends, your function delegate needs to be awaitable too, read: returns Task.

public abstract class BaseController<TController> : Controller where TController : Controller
{
    protected async Task<IActionResult> ExceptionHandledOperationAsync<T>(
        Func<Task<T>> operation,
        Func<object, IActionResult> successHandler,
        Func<IActionResult> exceptionHandler
    )
    {
        try
        {
            return successHandler.Invoke(await operation.Invoke());
        }
        catch (Exception exception)
        {
            //Logger.LogError($"Operation {Request.Path} Exception", exception);

            return exceptionHandler.Invoke();
        }
    }
}

[Route("api/[controller]")]
public class MyController : BaseController<MyController>
{
    [HttpGet]
    public async Task<IActionResult> Get()
    {
        return await ExceptionHandledOperationAsync(() => _someService.GetAsync(), Ok, NotFound);
    }
}

You'll need to move the successHandler (where you pass the result to Ok) into the method too like shown above. But it's really ugly code. Imho the SomeService should handle service failures itself and return null when it doesn't find any values.

Returning NotFound() on exception seems very odd, as it suggest that the record doesn't exist but it may have failed due to i.e. network connection or serialization of the data.

0
votes

I do agree with the comment above about not using Task.Run in an ASP.NET application. That being said, you can try putting a try / catch around your Invoke method.

Example:

 try
 {
     return await Task.Run(() =>
     {
         try
         {
              operation.Invoke();
         }
         catch (Exception ex)
         {
              // log exception
         }
     });
  }
  catch (Exception ex)
  {
      // captured SynchronizationContext
  }
0
votes

I was able to get this working after finally understanding what Tseng was telling me. This is how I changed it:

public abstract class BaseController<TController> : Controller where TController : Controller
{
    protected async Task<IActionResult> ExceptionHandledOperationAsync(Func<Task<IActionResult>> operation, Func<IActionResult> handleException)
    {
        try
        {
            return await operation.Invoke();
        }
        catch (Exception exception)
        {
            Logger.LogError($"Operation {Request.Path} Exception", exception);

            return handleException.Invoke();
        }
    }
}

And MyController like this:

[Route("api/[controller]")]
public class MyController : BaseController<MyController>
{
    [HttpGet]
    public async Task<IActionResult> Get()
    {
        return await ExceptionHandledOperationAsync(async () => Ok(await _someService.GetAsync()),
                                                    NotFound);
    }
}