Is it OK to catch Throwable for performing cleanup? [duplicate]

Question

Take an example like this:

public List<CloseableThing> readThings(List<File> files) throws IOException {
    ImmutableList.Builder<CloseableThing> things = ImmutableList.builder();
    try {
        for (File file : files) {
            things.add(readThing(file))
        }
        return things.build();
    } catch (Throwable t) {
        for (CloseableThing thing : things.build()) {
            thing.close();
        }
        throw t;
    }
}

A code review comment came in because there is generally a rule not to catch Throwable. The older pattern for doing this kind of failure-only cleanup was:

public List<CloseableThing> readThings(List<File> files) throws IOException {
    ImmutableList.Builder<CloseableThing> things = ImmutableList.builder();
    boolean success = false;
    try {
        for (File file : files) {
            things.add(readThing(file))
        }
        success = true;
        return things.build();
    } finally {
        if (!success) {
            for (CloseableThing thing : things.build()) {
                thing.close();
            }
        }
    }
}

I find this a bit messy and don't fully understand whether it's any different from catching the Throwable. In either case, the exception propagates. In either case, additional code is being run when potentially an OutOfMemoryError might have occurred.

So is finally really any safer?

Answer 1

Throwable is the parent type of Exception and Error, so catching Throwable means catching both Exceptions as well as Errors. An Exception is something you could recover (like IOException), an Error is something more serious and usually you could'nt recover easily (like ClassNotFoundError) so it doesn't make much sense to catch an Error unless you know what you're doing.

Answer 2

Is it OK to catch Throwable for performing cleanup?

In one word ... No.

The problem is that if you catch and rethrow Throwable, you have to declare that the method throws Throwable ... which is going to cause problems for anything that calls the method:

• The caller now has to "deal with" (probably propagate) the Throwable.
• The programmer now won't get any help from the compiler in the form of compiler errors about checked exceptions have not been dealt with. (When you "deal with" Throwable, that will include all checked and unchecked exceptions that haven't been already handled.)

Once you've started down this path, the throws Throwable will spread like a disease through the call hierarchy ...

---

The correct way to close resources is to use finally, or if you are coding for Java 7 or later, to use "try with resources", and make your resources auto-closable.

(In your example, that is a bit tricky, but you could extend an existing List class to make a "closeable list" class where the close() method closes all of the list members.

---

It is true that for Java 7 and later, you can get away with declaring the enclosing method as throwing only the checked exceptions that would be caught. However, catching Throwable to do cleanup is not what people expect to see. People expect to see a finally clause for cleanup. If you do it in a funky way, you are making your code harder to read ... and that is NOT "OK". Not even if your way is more concise.

And besides, your version won't compile with Java 6 and earlier.

---

In short, I agree with the reviewer of your code.

The only thing I would agree with is that if your version and the finally version are both "safe" assuming that they are implemented correctly. (The problem is that the programmer has to think harder in your case to realise that it is safe ... because of the non-idiomatic way you have coded it.)