12

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!

  • 4
    There is huge difference between wrapping each method in try/catch, logging the exception and letting the code chug along. And try/catch and rethrowing the exception with additional information. First is terrible practice. Second is perfectly fine way to improve your debuging experience. – Euphoric Feb 6 '18 at 11:19
  • I am saying try/catch each method and simply log in catch block and rethrow - is this ok to do ? – Rahul Agarwal Feb 6 '18 at 11:21
  • 1
    As pointed out by Amon. If your language has stack-traces, logging in each catch is pointless. But wrapping the exception and adding additional information is good practice. – Euphoric Feb 6 '18 at 11:23
  • 1
    See @Liath's answer. Any answer I gave would pretty much mirror his: catch exceptions as early as practicable and if all you can do at that stage is log some useful information, then do so and rethrow. Viewing this as an antipattern is nonsensical in my view. – David Arno Feb 6 '18 at 12:54
  • 1
    Liath : added small code snippet. I am using c# – Rahul Agarwal Feb 6 '18 at 13:20
11

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.

  • I liked "The problem isn't the local catch block, the problem is the log and rethrow" I think this makes perfect sense to ensure cleaner logging. But eventually it also means to be OK with try/catch being scattered across all methods, isn't it ? I think there must be some guideline to ensure this practice is followed judiciously rather than having every method doing so. – Rahul Agarwal Feb 7 '18 at 3:53
  • I provided a guideline in my answer. Does this not answer your questions? – Berin Loritsch Feb 7 '18 at 15:40
6

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.

  • 2
    What is point of logging in the catch, when the same logger would be used in the top-level exception handler? – Euphoric Feb 6 '18 at 11:25
  • You may have additional information (such as the local variables) which you will not have access to at the top of the stack. I'll update the example to illustrate. – Liath Feb 6 '18 at 11:27
  • 2
    In that case, throw new exception with additional data and inner exception. – Euphoric Feb 6 '18 at 11:27
  • 2
    @Euphoric yep, I've seen that done too, personally I'm not a fan however as it requires you to create new types of exception for almost every single method/scenario which I feel is a lot of overhead. Adding the log line here (and presumably another at the top) helps illustrate the flow of code when diagnosing issues – Liath Feb 6 '18 at 11:30
  • 1
    Java does not force you to handle the exception, it forces you to be aware of it. you can either catch it and do whatever or just declare it as something that function can throw and not do anything with it in the function.... Little nitpicking on an otherwise pretty good answer ! – Newtopian Feb 6 '18 at 15:38
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.

2

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.

  • Thanks Scott. Point you made "If you throw an exception with a meaningful message when a method receives an unexpected argument (and log it at the service boundary)" really strike and helped me to visualize situation hovering around me in context of method arguments. I think it makes sense to have safe guard clause and throw ArgumentException in that case rather than relying on catching and logging argument details – Rahul Agarwal Feb 7 '18 at 18:20
  • Scott, I've got the same feeling. Whenever I see log and rethow just to log the context, I feel like developers can not control the invariants of the class or can not safeguard the method call. An instead every method all the way up is wrapped in a similar try/catch/log/throw. And it's just terrible. – Max May 16 '18 at 22:29
2

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.

  • What about original exception type? If we wrap, it is gone. Is it a problem? Someone was relying on the SqlTimeoutException for instance. – Max May 16 '18 at 22:25
  • @Max: The original exception type is still available as the inner exception. – JacquesB May 17 '18 at 8:36
  • That's what I mean! Now everyone up the call stack, which were catching for the SqlException, will never get it. – Max May 17 '18 at 14:28
1

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.

  • 1
    "The exception itself should provide all the info necessary for proper logging, including message, error code, and what not" They should, but in practice, they don't Null reference exceptions being a classic case. I'm not aware of any language that will tell you the variable that caused it inside a complex expression , for example. – David Arno Feb 6 '18 at 12:28
  • 1
    @DavidArno True, but any context you could provide couldn't be that specific either. Otherwise you'd have try { tester.test(); } catch (NullPointerException e) { logger.error("Variable tester was null!"); }. The stack trace is sufficient in most cases, but lacking that, the type of error is usually adequate. – Neil Feb 6 '18 at 12:31

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.