ASP.NET REST controller with try-catch error handling

Question

Yesterday someone guided me through this article and I'm wondering whether I should use try-catch implementation or not? I have the following code below which is actually a RESTful service and I have implemented the try-catch pattern. I'm throwing errors only in debug mode just for testing purpose and during the release mode, I'm catching errors and returning the meaningful error messages such as Http Status Codes.

After reading this article, I'm confused whether I'm on the right track and if not, do I need to remove try-catch and throw errors instead of such meaningful messages to the end user?

Any code review/feedback is appreciated.

namespace NewEmployeeBuddy.Facade.Controllers.Employee
{
    [RoutePrefix("api/employee")]
    [RequireHttps]
    [BasicAuthorization]
    public class EmployeeController : ApiController
    {
        #region Properties
        private IEmployeeService _employeeService { get; set; }
        #endregion

        #region Constructor
        public EmployeeController()
        {
          //_employeeService = new EmployeeService();
        }

        public EmployeeController(IEmployeeService employeeService)
        {
            _employeeService = employeeService;
        }
        #endregion

        #region Actions
        [HttpPost, Route("add")]
        [ValidateRequest]
        public IHttpActionResult AddEmployee(AddEmployeeRequestDTO request)
        {
            try
            {
                if (request == null)
                    return Content(HttpStatusCode.BadRequest, ErrorCodes.E002);

                var response = _employeeService.AddEmployee(request);
                if (response.OperationResult)
                    return Content(HttpStatusCode.Created, response);

                return Content(HttpStatusCode.InternalServerError, ErrorCodes.E001);
            }
            catch (Exception ex)
            {
#if (DEBUG)
                Logger.Instance.Error(ex);
                throw ex;

#endif
                //Logger.Instance.Error(ex);
                //return Content(HttpStatusCode.InternalServerError, ErrorCodes.E001);
            }
        }
        #endregion
    }
}

Answer 1

throw ex;

This is a fatal mistake. It'll create a new stack-trace and you won't be able to tell where the actual exception occured. If you want to rethrow it then use just

throw;

without ex.

Answer 2

Typically in an ASP.NET app (and in fact in any app), you should only catch and handle the base System.Exception in a single place as close to the application entry point (or in this case request entry point) as possible. This means you don't have to duplicate exception handling code in every action method, and helps ensure you always return a consistent error response that is the same shape.

In .NET Core and OWIN based APIs this is typically done using a Middlware. With traditional ASP.NET APIs you can use an exception filter. The default MVC API template in VS2017 adds a System.Web.Mvc.FilterAttribute for you:

public class FilterConfig
{
    public static void RegisterGlobalFilters(GlobalFilterCollection filters)
    {
        filters.Add(new HandleErrorAttribute());
    }
}

You can replace this with your own implementation that for example logs the error using your chosen logging framework and forms the error response according to your API contract. Your controllers then do not need to worry about exception handling:

public class EmployeeController : ApiController
{
    private readonly IEmployeeService _employeeService;

    public EmployeeController(IEmployeeService employeeService)
    {
        _employeeService = employeeService;
    }

    [HttpPost, Route("add")]
    public IHttpActionResult Add(AddEmployeeRequest request)
    {
        var response = _employeeService.AddEmployee(request);

        if (!response.OperationResult)
        {
            // Can return content that matches what your error filter returns.
            return Content(HttpStatusCode.InternalServerError, new
            {
                Code = ErrorCodes.E002,
                Message = "Failed to add customer."
            });

            // Or could throw a custom exception that will be picked up in your error filter.
            throw new CustomerException("Failed to add customer.", ErrorCodes.E002);
        }

        return Ok(response);
    }
}

Answer 3

The linked post adds a neat summary:

So, to sum up:

• Don’t catch fatal exceptions; nothing you can do about them anyway, and trying to generally makes it worse.
• Fix your code so that it never triggers a boneheaded exception – an "index out of range" exception should never happen in production code.
• Avoid vexing exceptions whenever possible by calling the “Try” versions of those vexing methods that throw in non-exceptional circumstances. If you cannot avoid calling a vexing method, catch its vexing exceptions.
• Always handle exceptions that indicate unexpected exogenous conditions; generally it is not worthwhile or practical to anticipate every possible failure. Just try the operation and be prepared to handle the exception.

But then you conclude:

Yesterday someone guided me through this article and I'm wondering whether I should avoid try-catch completely ...

That's not in line with the summary.

or rarely use.

Depends on what you use with "rarely". If you mean to only use them when appropriate and avoid using them when not appropriate, then it's inherently good to do so (by definition of the word "appropriate").

---

Ask yourself why we avoid exceptions.

• Throw/catch logic is expensive, performance-wise.
• It can cause developers to use throw/catch logic as the intended flow of the application.
• When implemented too broadly, it can swallow exceptions that you needed to be aware of.

Which brings me to your code:

try
{
     //...
}
catch (Exception ex)
{
#if (DEBUG)
    Logger.Instance.Error(ex);
    throw ex;

#endif
    //Logger.Instance.Error(ex);
    //return Content(HttpStatusCode.InternalServerError, ErrorCodes.E001);
}

I infer that you're trying to catch boneheaded exceptions (as per the linked post's definition). You shouldn't usually be catching these. They should blow up in your face.

However, there is a bit lacking in your implementation.

• In debug, you log the exception and rethrow it. You're not handling it, you're basically intercepting an exception for logging purposes.
  • As t3chb0t mentioned, you'd be better off doing throw; instead of throw ex; so you conserve the stack trace, but that's a minor fix.
• In release, you catch the exception but you don't throw it again. You're effectively swallowing the exception and not alerting anyone than a problem was encountered.

For debug, the performance argument of throw/catch logic is moot. Debug builds are not optimized for performance, they are optimized for easy debugging. You're also not using it for logical flow, since you're only interested in more verbose logging. It is implemented rather broadly, but that seems reasonable for debugging purposes.
All in all, I'm not opposed to your approach that is specific to debug builds.

Your approach in release is not good for a few reasons:

• Don't swallow or hide exception. Instead, you should handle them gracefully; i.e. don't break the flow but rather intentionally pass an error message.
  • For the error messages, you should "dumb them down" for the end user. Instead of a nullreference exception message, simply report that "An error occurred on the server.
  • You could for example choose to show the actual exception message in debug mode (because you assume a developer is using the application then).
• You're catching an exception for no reason. Catching costs performance, yet you gain nothing from doing so.

A more graceful solution would be:

try
{
    //...
}
catch (Exception ex)
{
#if (DEBUG)
    Logger.Instance.Error(ex);
    throw;
#endif

    throw new Exception("An error occurred on the server.");
}

This assumes that the frontend is built in a way that it gracefully displays the error message to the user.

This handles both debug (detailed logging and message) and release (simplified message to the end user).

Note that you could log the real exception and give the user a reference ID that they can use if they create a support ticket:

catch (Exception ex)
{
    int errorReferenceNumber = Logger.Instance.Error(ex);

    #if (DEBUG)        
        throw;
    #endif

    throw new Exception(
            $"An error with reference {errorReferenceNumber} occurred on the server. Contact the helpdesk for help.");
}

This way, a release build still logs the error, hides it from the user, but still gives them a reference number (which does not reveal information about the error to the user) so that the user can give it to a developer who can examine the exception.