Arguments against error suppression

Question

I've found a piece of code like this in one of our projects:

    SomeClass QueryServer(string args)
    {
        try
        {
            return SomeClass.Parse(_server.Query(args));
        }
        catch (Exception)
        {
            return null;
        }
    }

As far as I understand, suppressing errors like this is a bad practice, as it destroys useful information from the original server's exception and makes the code continue when it actually should terminate.

When is it appropriate to completely suppress all errors like this?

Answer 1

Imagine code with thousands files using a bunch of libraries. Imagine all of them are coded like this.

Imagine, for example, an update of your server causes one configuration file disappear; and now all you have is a stack trace is a null pointer exception when you try using that class: how would you resolve that? It could take hours, where at least just logging the raw stack trace of the file not found [file path] may enable you to resolve momentarily.

Or even worse: a failure in one off the libraries you use after an update that makes your code crash later on. How can you track this back to the library ?

Even without robust error handling just doing

throw new IllegalStateException("THIS SHOULD NOT HAPPENING")

or

LOGGER.error("[method name]/[arguments] should not be there")

may save you hours of time.

But there are some cases where you might really want to ignore the exception and return null like this (or do nothing). Especially if you integrate with some badly designed legacy code and this exception is expected as a normal case.

In fact when doing this you should just wonder if you really are ignoring the exception or "properly handling" for your needs. If returning null is "properly handling" your exception in that given case, then do it. And add a comment why this is the proper thing to do.

Best practices are things to follow in most of cases, maybe 80%, maybe 99%, but you'll always find one edge case where they don't apply. In that case, leave a comment why you're not following the practice for the others (or even yourself) who will read your code months later.

Answer 2

There are cases where this pattern is useful - but they are typically used when the exception generated should never have been (ie when exceptions are used for normal behaviour).

For example, imagine you have a class that opens a file, stores it in the object that is returned. If the file does not exist, you may consider that to not be an error case, you return null and let the user create a new file instead. The file-open method may throw an exception here to indicate no file present, so silently catching it may be a valid situation.

However, its not often you'd want to do this, and if the code is littered with such a pattern you'd want to deal with it now. At the very least I'd expect such a silent catch to write a log line saying this is what has happened (you have tracing, right) and a comment to explain this behaviour.

Answer 3

This is 100% context dependent. If the caller of such function has the requirement to display or log errors somewhere, then it is obviously nonsense. If the caller ignored all errors or exception messages either, it would not make much sense to return the exception or its message. When the requirement is to make the code only terminate without displaying any reason, then a call

   if(QueryServer(args)==null)
      TerminateProgram();

would probably be sufficient. You have to consider if this will make it hard to find the cause for the error by the user - if that is the case, this is the wrong form of error handling. However, if the calling code looks like this

  result = QueryServer(args);
  if(result!=null)
      DoSomethingMeaningful(result);
  // else ??? Mask the error and let the user run into trouble later

then you should have an argument with the developer during code review. If someone implements such a form of non-error handling just because he is too lazy to find out about the correct requirements, then this code should not go into production, and the author of the code should be taught about the possible consequences of such laziness.

Answer 4

In the years I have spent programming and developing systems, there are only two situations where I found the pattern in question useful (in both cases the supression contained also logging of the thrown exception, I do not consider plain catch and null return as a good practice).

The two situations are the following:

1. When the exception was not considered an exceptional state

This is when you do an operation on some data, which may throw, you know it may throw but you still want your application to keep running, because you do not need the processed data. If you receive them, it's good, if you do not, it is also good.

Some optional attributes of a class may come in mind.

2. When you're providing a new (better, faster?) implementation of a library using an interface already used in an application

Imagine you have an application using some sort of an old library, which did not throw exceptions but returned null on error. So you created an adapter for this library, pretty much copying the original API of the library, and are using this new (still non-throwing) interface in your application and handling the null checks yourself.

A new version of the library comes, or perhaps a completely different library offering the same functionality, which, instead of returning nulls, throws exceptions and you want to use it.

You do not want to leak the exceptions to your main application, so you supress and log them in the adapter you create to wrap this new dependency.

---

The first case is not an issue, it is the desired behaviour of the code. In the second situation, however, if everywhere the null return value of the library adapter really means an error, refactoring the API to throw an exception and catching it instead of checking for null may be (and code-wise usually is) a good idea.

I personally use exception supression only for the first case. I have only used it for the second case, when we did not have the budget to make the rest of the application work with the exceptions instead of nulls.

Answer 5

While it seems logical to say that programs should only catch exceptions they know how to handle, and can't possibly know how to handle exceptions which weren't anticipated by the programmer, such a claim ignores the fact that many operations can fail in a nearly limitless number of ways which have no side-effects, and that in many cases the proper handling for the vast majority of such failures will be identical; the exact details of the failure will be irrelevant, and consequently it shouldn't matter whether the programmer anticipated them.

If, for example, the purpose of a function is to read a document file into a suitable object and either create a new document window to show that object or report to the user that the file couldn't be read, an attempt to load an invalid document file shouldn't crash the application--it should instead show a message indicating the problem but let the rest of the application continue running normally unless for some reason the attempt to load the document has corrupted the state of something else in the system.

Fundamentally, the proper handling of an exception will often depend less upon the exception type than the location where it was thrown; if a resource is guarded by a read-write lock, and an exception is thrown within a method that has acquired the lock for reading, the proper behaviour should generally be to release the lock since the method can't have done anything to the resource. If an exception is thrown while the lock is acquired for writing, the lock should often be invalidated, since the guarded resource may be in an invalid state; if the locking primitive doesn't have an "invalidated" state, one should add a flag to track such invalidation. Releasing the lock without invalidating it is bad because other code may see the guarded object in an invalid state. Leaving the lock dangling, however, isn't a proper solution either. The right solution is to invalidate the lock so that any pending or future attempts at acquisition will immediately fail.

If it turns out that an invalidate resource gets abandoned before there's any attempt to use it, there's no reason it should bring down the application. If an invalidated resource is critical to an application's continued operation, the application will need to be brought down but invalidating the resource will likely make that happen. The code which received the original exception will often have no way of knowing which situation applies, but if it invalidates the resource it can ensure that the correct action will end up being taken in either case.

Answer 6

Swallowing this sort of error isn't particularly useful to anyone. Yes, the original caller may not care about the exception but someone else might.

Then what do they do? They'll add code to handle the exception to see what flavour of exception they're getting back. Great. But if the exception handler is left in, the original caller no longer gets null back and something breaks elsewhere.

Even if you're aware that some upstream code could throw an error and thus return null, it would be seriously inert of you not to at least try to head off the exception in the calling code IMHO.

Answer 7

I've seen examples where third-party libraries had potentially useful methods, except that they throw exceptions in some cases that should work for me. What can I do?

• Implement exactly what I need myself. Can be difficult on a deadline.
• Modify the third-party code. But then I have to look for merge conflicts with every upgrade.
• Write a wrapper method to turn the exception into an ordinary null pointer, a boolean false, or whatever.

For example, the library method

public foo findFoo() {...} 

returns the first foo, but if there is none it throws an exception. But what I need is a method to return the first foo or a null. So I write this one:

public foo myFindFoo() {
    try {
        return findFoo() 
    } 
    catch (NoFooException ex) {
        return null;
    }
}

Not neat. But sometimes the pragmatic solution.