Try/Catch/Log/Rethrow - Is Anti Pattern?

Question

I can see several post where importance of handling exception at central location or at process boundary been emphasized as a good practice rather than littering every code block around try/catch. I strongly believe that most of us understand the importance of it however i see people still ending up with catch-log-rethrow anti pattern mainly because to ease out troubleshooting during any exception, they want to log more context specific information (example : method parameters passed) and the way is to wrap method around try/catch/log/rethrow.

public static bool DoOperation(int num1, int num2)
{
    try
    {
        /* do some work with num1 and num2 */
    }
    catch (Exception ex)
    {
        logger.log("error occured while number 1 = {num1} and number 2 = {num2}"); 
        throw;
    }
}

Is there right way to achieve this while still maintaining exception handling good practice ? I heard of AOP framework like PostSharp for this but would like to know if there is any downside or major performance cost associated with these AOP frameworks.

Thanks!

Answer 1

The problem isn't the local catch block, the problem is the log and rethrow. Either handle the exception or wrap it with a new exception that adds additional context and throw that. Otherwise you will run into several duplicate log entries for the same exception.

The idea here is to enhance the ability to debug your application.

Example #1: Handle it

try
{
    doSomething();
}
catch (Exception e)
{
    log.Info("Couldn't do something", e);
    doSomethingElse();
}

If you handle the exception, you can easily downgrade the importance of the exception log entry and there is no reason to percolate that exception up the chain. It's already dealt with.

Handling an exception can include informing users that a problem happened, logging the event, or simply ignoring it.

NOTE: if you intentionally ignore an exception I recommend providing a comment in the empty catch clause that clearly indicates why. This lets future maintainers know that it was not a mistake or lazy programming. Example:

try
{
    context.DrawLine(x1,y1, x2,y2);
}
catch (OutOfMemoryException)
{
    // WinForms throws OutOfMemory if the figure you are attempting to
    // draw takes up less than one pixel (true story)
}

Example #2: Add additional context and throw

try
{
    doSomething(line);
}
catch (Exception e)
{
    throw new MyApplicationException(filename, line, e);
}

Adding additional context (like the line number and file name in parsing code) can help enhance the ability to debug input files--assuming the problem is there. This is kind of a special case, so re-wrapping an exception in an "ApplicationException" just to rebrand it doesn't help you debug. Make sure you add additional information.

Example #3: Don't do anything with the exception

try
{
    doSomething();
}
finally
{
   // cleanup resources but let the exception percolate
}

In this final case, you just allow the exception to leave without touching it. The exception handler at the outermost layer can handle the logging. The finally clause is used to make sure any resources needed by your method are cleaned up, but this is not the place to log that the exception was thrown.

Answer 2

I don't believe local catches are an anti-pattern, in fact if I remember correctly it's actually enforced in Java!

What's key for me when implementing error handling is the overall strategy. You may want a filter which catches all exceptions at the service boundary, you may want to manually intercept them - both are fine as long as there is an overall strategy, that will fall into your teams' coding standards.

Personally I like to catch errors inside a function when I can do one of the following:

• Add contextual information (such as the state of objects or what was going on)
• Handle the exception safely (such as a TryX method)
• Your system is crossing a service boundary and calling into an external library or API
• You want to catch and rethrow a different type of exception (perhaps with the original as an inner exception)
• The exception was thrown as part of some low value background functionality

If it's not one of these cases I don't add a local try/catch. If it is, depending on the scenario I may handle the exception (for example a TryX method which returns a false) or rethrow so the exception will be handled by the global strategy.

For example:

public bool TryConnectToDatabase()
{
  try
  {
    this.ConnectToDatabase(_databaseType); // this method will throw if it fails to connect
    return true;
  }
  catch(Exception ex)
  {
     this.Logger.Error(ex, "There was an error connecting to the database, the databaseType was {0}", _databaseType);
    return false;
  }
}

Or a rethrow example:

public IDbConnection ConnectToDatabase()
{
  try
  {
    // connect to the database and return the connection, will throw if the connection cannot be made
  }
  catch(Exception ex)
  {
     this.Logger.Error(ex, "There was an error connecting to the database, the databaseType was {0}", _databaseType);
    throw;
  }
}

Then you catch the error at the top of the stack and present a nice user friendly message to the user.

Whichever approach you take it's always worth creating unit tests for this scenarios so you can make sure the functionality doesn't change and disrupt the project's flow at a later date.

You've not mentioned which language you're working in but being a .NET developer and having seen this too many times not to mention it.

DO NOT WRITE:

catch(Exception ex)
{
  throw ex;
}

Use:

catch(Exception ex)
{
  throw;
}

The former resets the stack trace and makes your top level catch utterly useless!

TLDR

Catching locally isn't an anti-pattern, it can often be part of a design and can help to add additional context to the error.

Answer 3

This depends a lot on the language. E.g. C++ doesn't offer stack traces in exception error messages, so tracing the exception through frequent catch–log–rethrow can be helpful. In contrast, Java and similar languages offer very good stack traces, though the format of these stack traces may not be very configurable. Catching and rethrowing exceptions in these languages is utterly pointless unless you can really add some important context (e.g. connecting a low-level SQL exception with the context of a business logic operation).

Any error handling strategy that is implemented through reflection is almost necessarily less efficient than functionality that's built into the language. Additionally, pervasive logging has unavoidable performance overhead. So you do need to balance the stream of information you get against other requirements for this software. That said, solutions like PostSharp that are built on compiler-level instrumentation will generally do a lot better than run-time reflection.

I personally believe that logging everything is not helpful, because it includes loads of irrelevant information. I'm therefore sceptical of automated solutions. Given a good logging framework, it might be sufficient to have an agreed-upon coding guideline that discusses what kind of information you would like to log and how this info should be formatted. You can then add logging where it matters.

Logging on business logic is much more important than logging on utility functions. And collecting stack traces of real-world crash reports (which only requires logging at the top level of a process) allows you to locate areas of the code where logging would have the most value.

Answer 4

When I see try/catch/log in every method it raises concerns that the developers had no idea what might or might not happen in their application, assumed the worst, and preemptively logged everything everywhere because of all the bugs they were expecting.

This is a symptom that unit and integration testing are insufficient and developers are accustomed to stepping through lots of code in the debugger and are hoping that somehow lots of logging will allow them to deploy buggy code in a test environment and find the problems by looking at the logs.

Code that throws exceptions can be more useful than redundant code that catches and logs exceptions. If you throw an exception with a meaningful message when a method receives an unexpected argument (and log it at the service boundary) it's much more helpful than immediately logging the exception thrown as a side effect of the invalid argument and having to guess what caused it.

Nulls are an example. If you get a value as an argument or the result of a method call and it shouldn't be null, throw the exception then. Don't just log the resulting NullReferenceException thrown five lines later because of the null value. Either way you get an exception, but one tells you something while the other makes you look for something.

As said by others, it's best to log exceptions at the service boundary, or whenever an exception isn't rethrown because it's handled gracefully. The most important difference is between something and nothing. If your exceptions are logged in one place that's easy to get to you'll find the information you need when you need it.

Answer 5

If you need to record context information which is not already in the exception, you wrap it in a new exception, and provide the original exception as InnerException. That way you still have the original stack trace preserved. So:

public static bool DoOperation(int num1, int num2)
{
    try
    {
        /* do some work with num1 and num2 */
    }
    catch (Exception ex)
    {
        throw new Exception("error occured while number 1 = {num1} and number 2 = {num2}", ex);
    }
}

The second parameter to the Exception constructor provides an inner exception. Then you can log all exceptions in a single place, and you still get the full stack trace and the contextual information in the same log entry.

You might want to use a custom exception class, but the point is the same.

try/catch/log/rethrow is a mess because it will lead to confusing logs - e.g. what if a different exception happens in another thread in between logging the contextual information and logging the actual exception at in the top-level handler? try/catch/throw is fine though, if the new exception adds information to the original.

Answer 6

The exception itself should provide all the info necessary for proper logging, including message, error code, and what not. Therefore there should be no need to catch an exception only to rethrow it or throw a different exception.

Often you'll see the pattern of several exceptions caught and rethrown as a common exception, such as catching a DatabaseConnectionException, a InvalidQueryException, and a InvalidSQLParameterException and rethrowing a DatabaseException. Though to this, I would argue that all these specific exceptions should derive from DatabaseException in the first place, so rethrowing is not necessary.

You'll find that removing unnecessary try catch clauses (even the ones for purely logging purposes) will actually make the job easier, not harder. Only the places in your program which handle the exception should be logging the exception, and, should all else fail, a program-wide exception handler for one final attempt at logging the exception before gracefully exiting the program. The exception should have a full stack trace indicating the exact point in which the exception was thrown, so it often isn't necessary to provide "context" logging.

That said AOP may be a quick fix solution for you, though it usually entails a slight slowdown overall. I would encourage you to instead remove the unnecessary try catch clauses entirely where nothing of value is added.