Why catch specific Java exceptions unless specific handling is needed?-CodePudding

Here is the code taken from https://dzone.com/articles/9-best-practices-to-handle-exceptions-in-java

public void automaticallyCloseResource() {
    File file = new File("./tmp.txt");
    try (FileInputStream inputStream = new FileInputStream(file);) {
        // use the inputStream to read a file

    } catch (FileNotFoundException e) {
        log.error(e);
    } catch (IOException e) {
        log.error(e);
    }
}

If all the catch handlers are essentially doing the same, I don't see why we catch each specific exception separately instead of simply catching the generic base Exception class.

Essentially, if you are not looking for properties/methods/anything of the specific exception, why not just catch the generic Exception?

CodePudding user response：

I assume the writer of the article is simply trying to make a point of catching the most specific exceptions first, then the more generic ones. This is not just a best practice, but a Java language requirement as the code won't compile otherwise due to having defined unreachable code blocks.

In the particular code block quoted by you, it's not necessary and you might as well just catch IOException, since FileNotFoundException extends it the behaviour to be expected once an exception is caught is exactly the same.

CodePudding user response：

You're right that this is pointless as written.

They should probably have written log.error("TODO: Handle FileNotFound", e); or // TODO: Handle missing files so that you wouldn't be tempted to think that log.error(e); is the end goal rather than a stand-in.

It makes more sense when you look at the suggested rewrite:

public void closeResourceInFinally() {
    FileInputStream inputStream = null;
    try {
        File file = new File("./tmp.txt");
        inputStream = new FileInputStream(file);

        // use the inputStream to read a file

    } catch (FileNotFoundException e) {
        log.error(e);
    } finally {
        if (inputStream != null) {
            try {
                inputStream.close();
            } catch (IOException e) {
                log.error(e);
            }
        }
    }
}

It appears that the author wanted to catch a FileNotFoundException while initializing, and an IOException while closing.

If the first example had only caught the IOException because it would amount to the same, wouldn't you have asked "wait, why does it suddenly catch FileNotFoundException now?"

By including both in the original example, it's easier to see how the first maps to the second.

CodePudding user response：

Given that in that particular example there is no difference in handling exceptions, there is no utility to make any differnce between them. So catching only IOException would be just fine (as FileNotFoundException is IOException subtype).

Now, in general, if you do not know how to handle checked exceptions, the thing you should do is to rethrown them as runtime exception. Don't even bother to log them unless you are able to add specific cause of the exception. Like this:

public void automaticallyCloseResource() {
    File file = new File("./tmp.txt");
    try (FileInputStream inputStream = new FileInputStream(file);) {
        // use the inputStream to read a file
    }
    catch (IOException e) {
       throw new RuntimeException("An error handling './tmp.txt'", e);
    }
}

So what is the logic in this?

First we do not rethrow cause exception (function doesn't have "throws IOException" clause). Logic being - we do not know to handle this IOException - so any caller of our method is going to have ever less knowledge how to handle it. So don't bother people with unnecessary boiler plate.

And who is going to handle this RuntimeException?

Well every thread in your control should have a top level exception handler. Nothing smart, just a generic handler:

try {
  // my app code here
} 
catch(MyCustomException exc) {
  // do smart stuff here
}
catch(RuntimeExceptio exc) {
  log.error("An unexpected has happened", exc);
  // rethrow or consume here
}

As you can see, logging of "unrecoverable" exceptions is handled at highest catch level. Logic being that logging exception and than rethrowing it will only lead to logs being polluted with same exception multiple times. Given that all Java exceptions carry perfect information where they were raised, unless you can add useful stuff to log, don't bother to log them except on top level.

And finally why shouldn't you catch all Exceptions or Throwables?

Again, as general advice, your code should catch (and rethrow) only exceptions which are directly raised by code you invoked. Generic Exception/Throwable block will catch all other exceptions which you have no chance to handle properly. System stuff like OutOfMemoryException. Those exceptions are unrecoverable and you tempting with them will only lead to your application being less stable and harder to debug.

CodePudding user response：

For the given sample, having two catch blocks is really redundant, yes. It could easily handled in one single catch block for IOException.

But let's assume with have a method like this:

public final Optional<Status> doSomething( final String... arguments )
  throws IOException, SQLException, PatternSyntaxException
{
  …
}

We don't care what the method is really doing, we just assume that it will not check the arguments for null – meaning we can see a NullPointerException under some circumstances.

Let's use that method now, with the error handling that you suggest:

try
{
  final var status = doSomething( arguments ).get();  
}
catch( final Exception e ) // Not good!
{
  handleExpectedException( e );
}

Looks perfectly ok – but handleExcpectedException() may be called with some 'unexpected' exceptions, too: with a NullPointerException or with a NoSuchElementException.

Ok, second attempt:

try
{
  final var status = doSomething( arguments ).get();  
}
catch( final IOException e )
{
  handleExpectedException( e );
}
catch( final SQLException e )
{
  handleExpectedException( e );
}
catch( final PatternSyntaxException e )
{
  handleExpectedException( e );
}
catch( final NullPointerException e )
{
  handleUnexpectedException( e );
}
catch( final NoSuchElementException e )
{
  handleUnexpectedException( e );
}

Confessed, that's a lot of lines, for nearly nothing. Therefore Java 5 (or Java 6?) introduced a way to combine the catch for multiple exceptions that are handled the same way:

try
{
  final var status = doSomething( arguments ).get();  
}
catch( final IOException | SQLException | PatternSyntaxException e )
{
  handleExpectedException( e );
}
catch( final NullPointerException | NoSuchElementException e )
{
  handleUnexpectedException( e );
}

But why do we want to list the exceptions? Why not just having two categories and handle them like this:

try
{
  final var status = doSomething( arguments ).get();  
}
catch( final RuntimeException e )
{
  handleUnexpectedException( e );
}
catch( final Exception e )
{
  handleExpectedException( e );
}

That's because we want that unexpected exceptions will bubble up when the occur! We want (at least, I want) that a program, that behaves completely unexpected, will fail fast! And loud!

doSomething() tells us, that it may throw three different types of exceptions (and usually, the respective documentation will tell us, why). So we can prepare our code to handle them. Just log them and terminate is the easiest way, but perhaps we know how to recover from the error condition and can retry, or we have an alternative processing, or whatever.

But the NullPointerException either indicates that there is a programming bug in the code of doSomething(), or we have not read the documentation properly and fed a null into it. Both nothing that can be fixed during runtime.

And the NoSuchElementException is clearly caused by a bug in our code: doSomething() returns an instance of Optional and that can be empty; calling Optional::get on the result without checking it is definitely a bug! And I want to see this bug soon, in order to be able to fix it soon!

A catch block for Exception may swallow these exceptions and they will never surface – and I will never find the reason why may program returns sometimes unexpected results …

So the recommendation is to catch only those exceptions that are expected at this level (either they are declared with a throws clause or described in the documentation) and that could be handled in the current catch block.

To 'handle' an exception means that the system is brought back in a stable state!

Logging a FileNotFoundException in the catch block and then continuing after that catch block like the (non-existing!) file was found and opened is calling for desaster!

If you just catch an exception locally because you want to enrich the error message, you should not log it; just wrap it and re-throw:

…
catch( final FileNotFoundException e )
{
  final var message = "MatrixReport output file %s could not be found!".formatted( e.getMessage() );
  throw MyApplicationError( message, e );
}

So finally, when I would use doSomething() (without fixing the already mentioned bugs), my code max look like this (assuming that the FileNotFoundException is documented):

try
{
  final var status = doSomething( arguments ).get();  
}
catch( final FileNotFoundException e )
{
  final var message = "MatrixReport output file %s could not be found!".formatted( e.getMessage() );
  throw MyApplicationError( message, e );
}
catch( final IOException | SQLException | PatternSyntaxException e )
{
  handleExpectedException( e );
}

Ok, we handle only exceptions that we can handle, and let the rest bubbling up … but who will now take care of the these exceptions?

When this happens in the main thread, the exception that terminates it will be printed to stdout. Great! And what about the other threads?

Usually, these will die silently …

So you quite often see the recommendations to have a catch block for RuntimeException, like this:

public final void run()
{
  try
  {
    // Do this thread's work …
  }
  catch( final RuntimeException e )
  {
    log.error( e );
  }
}

But this can be achieved easier: you can set an UncaughtExceptionHandler instance to the Thread! See the JavaDoc. You can do this even implicitly by assigning the thread to a specific ThreadGroup that provides an UncaughtExceptionHandler (you have to implement your own class that extends ThreadGroup for that), or you have a ThreadFactory that does the assigning for you …

Oh, before I forget: I introduced handleExpectedException() for a reason, instead of simply using log.error(): if you do not fix the status that is indicated by the exception, you may not continue the regular flow of the code! Logging an exception is not handling it!

This means handleException() will not return regularly in most cases; perhaps is throws an instance of MyApplicationError, too, or it will call System::exit (really nasty when you are in a web application … don't do it!) or whatever is appropriate …