Code Review #1: Masking of Throwables

When it comes to finally blocks, extra care is required. The code within a finally block can make very few assumptions about the state of variables that it is dealing with. Usually where developers slip-up is with null handling. This error is compounded by finally block lacking the try block exception management protection.

An otherwise well-written method with regards to correct try/catch blocks can still fail badly when it comes to the finally block code. A method that handles all exceptions can still throw an exception from the finally block itself. Usually the finally block throws an exception when try or catch block code threw an exception.

This code has this problem:

   private void copyFile(String fileName) throws IOException {
        File destFile = new File(archiveDirectory, fileName);
        FileChannel srcChannel = null;
        FileChannel dstChannel = null;
        try {
            srcChannel = new FileInputStream(new File(workingDirectory,
                    fileName)).getChannel();
            dstChannel = new FileOutputStream(destFile.getAbsolutePath())
                    .getChannel();
            dstChannel.transferFrom(srcChannel, 0, srcChannel.size());
            destFile = new File(archiveDirectory, fileName+".sha");
            srcChannel = new FileInputStream(new File(workingDirectory,
                    fileName+".sha")).getChannel();
            dstChannel = new FileOutputStream(destFile.getAbsolutePath())
                    .getChannel();
            dstChannel.transferFrom(srcChannel, 0, srcChannel.size());
        } finally {
            srcChannel.close();
            dstChannel.close();
        }
    }

Did you spot the problem?

What happens if either of the first two lines throws an exception?

• srcChannel or dstChannel will be null.
• A NullPointerException(NPE) will be thrown from the finally block.
• The NPE will *mask* the original exception.

This last point bears repeating. The original cause of the problem will be lost! Now you as a developer may assume that the lost exception will be an IOException and 99% of the time especially during development this will be correct.

But many developers forget that there are a few externally generated events that can cause throwables to be thrown. Thats right! The root for “things that can be thrown” is java.lang.Throwable – not java.lang.Exception!

Some non-Exception examples include: StackOverflowError and OutOfMemoryError.

So the masking NPE could easily hide an OutOfMemoryError, which results in a lot of hair pulling when:

• the exception is only occurring occasionally…
• only on production boxes…
• at a customer site…
• they will not let you update the code until you have a fix…
• the customer is your company’s largest account…
• and their CEO is calling your CEO…

It may not play out like this in all its glory. But take the words to heart and make sure you never mask Throwables!