Is there a preference for nested try/catch blocks?

Question

One of the things that always bugs me about using Readers and Streams in Java is that the close() method can throw an exception. Since it's a good idea to put the close method in a finally block, that necessitates a bit of an awkward situation. I usually use this construction:

FileReader fr = new FileReader("SomeFile.txt");
try {
    try {
        fr.read();
    } finally {
        fr.close();
    }
} catch(Exception e) {
    // Do exception handling
}

But I've also seen this construction:

FileReader fr = new FileReader("SomeFile.txt");
try {
    fr.read() 
} catch (Exception e)
    // Do exception handling
} finally {
    try {
        fr.close();
    } catch (Exception e)
        // Do exception handling
    }
}

I prefer the first construction because there's only one catch block and it just seems more elegant. Is there a reason to actually prefer the second or an alternate construction?

UPDATE: Would it make a difference if I pointed out that both read and close only throw IOExceptions? So it seems likely to me that, if read fails, close will fail for the same reason.

Answer 1

The difference, as far as I can see, is that there are different exceptions and causes in play on different levels, and the

catch (Exception e)

obscures that. The only point of the multiple levels is to distinguish your exceptions, and what you'll do about them:

try
{
  try{
   ...
  }
   catch(IOException e)
  {
  ..
  }
}
catch(Exception e)
{
  // we could read, but now something else is broken 
  ...
}

Answer 2

I'm afraid there's a big problem with the first example, which is that if an exception happens on or after the read, the finally block executes. So far so good. But what if the fr.close() then causes another exception to be thrown? This will "trump" the first exception (a bit like putting return in a finally block) and you will lose all information about what actually caused the problem to begin with.

Your finally block should use:

IOUtil.closeSilently(fr);

where this utility method just does:

public static void closeSilently(Closeable c) {
    try { c.close(); } catch (Exception e) {} 
}

Answer 3

I prefer the second one. Why? If both read() and close() throw exceptions, one of them could be lost. In the first construction, the exception from close() overrides the exception from read(), while in the second one, the exception from close() is handled separately.

EDIT: I type too slowly.

Answer 4

The standard convention I use is that you must not let exceptions escape a finally block.

This is because if an exception is already propagating the exception thrown out of the finally block will trump the original exception (and thus be lost).

In 99% of cases this is not what you want as the original exception is probably the source of your problem (any secondary exceptions may be side effects from the first but will obscure your ability to find the source of the original exception and thus the real problem).

So your basic code should look like this:

try
{
    // Code
}
// Exception handling
finally
{
    // Exception handling that is garanteed not to throw.
    try
    {
         // Exception handling that may throw.
    }
    // Optional Exception handling that should not throw
    finally()
    {}
}

Answer 5

I would always go for the first.

If close were to throw an exception (in practice that will never happen for a FileReader), wouldn't the standard way of handling that be to throw an exception appropriate to the caller? The close exception almost certainly trumps any problem you ha dusing the resource. The second method is probably more appropriate if your idea of exception handling is to call System.err.println.

There is an issue of how far exceptions should be thrown. ThreadDeath should always be rethrown, but any exception within finally would stop that. Similarly Error should throw further than RuntimeException and RuntimeException further than checked exceptions. If you really wanted to you could write code to follow these rules, and then abstract it with the "execute around" idiom.

Answer 6

If both read and close throw an exception, the exception from read will be hidden in option 1. So, the second option does more error handling.

However, in most cases, the first option will still be preferred.

1. In many cases, you can't deal with exceptions in the method they are generated, but you still must encapsulate the stream handling within that operation.
2. Try adding a writer to the code and see how verbose the second approach gets.

If you need to pass all the generated exceptions, it can be done.

Answer 7

I usually do the following. First, define a template-method based class to deal with the try/catch mess

import java.io.Closeable;
import java.io.IOException;
import java.util.LinkedList;
import java.util.List;

public abstract class AutoFileCloser {
    private static final Closeable NEW_FILE = new Closeable() {
        public void close() throws IOException {
            // do nothing
        }
    };

    // the core action code that the implementer wants to run
    protected abstract void doWork() throws Throwable;

    // track a list of closeable thingies to close when finished
    private List<Closeable> closeables_ = new LinkedList<Closeable>();

    // mark a new file
    protected void newFile() {
        closeables_.add(0, NEW_FILE);
    }

    // give the implementer a way to track things to close
    // assumes this is called in order for nested closeables,
    // inner-most to outer-most
    protected void watch(Closeable closeable) {
        closeables_.add(0, closeable);
    }

    public AutoFileCloser() {
        // a variable to track a "meaningful" exception, in case
        // a close() throws an exception
        Throwable pending = null;

        try {
            doWork(); // do the real work

        } catch (Throwable throwable) {
            pending = throwable;

        } finally {
            // close the watched streams
            boolean skip = false;
            for (Closeable closeable : closeables_) {
                if (closeable == NEW_FILE) {
                    skip = false;
                } else  if (!skip && closeable != null) {
                    try {
                        closeable.close();
                        // don't try to re-close nested closeables
                        skip = true;
                    } catch (Throwable throwable) {
                        if (pending == null) {
                            pending = throwable;
                        }
                    }
                }
            }

            // if we had a pending exception, rethrow it
            // this is necessary b/c the close can throw an
            // exception, which would remove the pending
            // status of any exception thrown in the try block
            if (pending != null) {
                if (pending instanceof RuntimeException) {
                    throw (RuntimeException) pending;
                } else {
                    throw new RuntimeException(pending);
                }
            }
        }
    }
}

Note the "pending" exception -- this takes care of the case where an exception thrown during close would mask an exception we might really care about.

The finally tries to close from the outside of any decorated stream first, so if you had a BufferedWriter wrapping a FileWriter, we try to close the BuffereredWriter first, and if that fails, still try to close the FileWriter itself.

You can use the above class as follows:

try {
    // ...

    new AutoFileCloser() {
        @Override protected void doWork() throws Throwable {
            // declare variables for the readers and "watch" them
            FileReader fileReader = null;
            BufferedReader bufferedReader = null;
            watch(fileReader = new FileReader("somefile"));
            watch(bufferedReader = new BufferedReader(fileReader));

            // ... do something with bufferedReader

            // if you need more than one reader or writer
            newFile(); // puts a flag in the 
            FileWriter fileWriter = null;
            BufferedWriter bufferedWriter = null;
            watch(fileWriter = new FileWriter("someOtherFile"));
            watch(bufferedWriter = new BufferedWriter(fileWriter));

            // ... do something with bufferedWriter
        }
    };

    // .. other logic, maybe more AutoFileClosers

} catch (RuntimeException e) {
    // report or log the exception
}

Using this approach you never have to worry about the try/catch/finally to deal with closing files again.

If this is too heavy for your use, at least think about following the try/catch and the "pending" variable approach it uses.

Answer 8

2nd approach.

Otherwise, I don't see you catching the exception from the FileReader constructor

http://java.sun.com/j2se/1.5.0/docs/api/java/io/FileReader.html#FileReader(java.lang.String)

public FileReader(String fileName) throws FileNotFoundException

So, I usually have the constructor inside the try block as well. the finally block checks to see if the reader is NOT null before trying to do a close.

The same pattern goes for Datasource, Connection, Statement, ResultSet.

Answer 9

Sometimes nested try-catch is not a preference, consider this:

try{
 string s = File.Open("myfile").ReadToEnd(); // my file has a bunch of numbers
 // I want to get a total of the numbers 
 int total = 0;
 foreach(string line in s.split("\r\n")){
   try{ 
     total += int.Parse(line); 
   } catch{}
 }
catch{}

This is probably a bad example, but there are times you will need nested try-cactch.

Answer 10

I like the approach by @Chris Marshall, but I never like to see exceptions getting swallowed silently. I think its best to log exceptions, especially if you are contiuing regardless.

I always use a utility class to handle these sort of common exceptions, but I would make this tiny different to his answer.

I would always use a logger (log4j for me) to log errors etc.

IOUtil.close(fr);

A slight modification to the utility method:

public static void close(Closeable c) {
    try {
      c.close();
    } catch (Exception e) {
      logger.error("An error occurred while closing. Continuing regardless", e); 
    } 
}