Is it OK to catch Throwable for performing cleanup?

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.)

Answer 3

This is an attempt to answer my own question, but it uses experiment and the results of what comes out of the Java compiler, so it isn't particularly addressing the philosophy or anything like that.

Here is some sample code for catch-cleanup-and-rethrow:

public CompoundResource catchThrowable() throws Exception {
    InputStream stream1 = null;
    InputStream stream2 = null;
    try {
        stream1 = new FileInputStream("1");
        stream2 = new FileInputStream("2");
        return new CompoundResource(stream1, stream2);
    } catch (Throwable t) {
        if (stream2 != null) {
            stream2.close();
        }
        if (stream1 != null) {
            stream1.close();
        }
        throw t;
    }
}

That compiles to the following bytecode:

public Exceptions$CompoundResource catchThrowable() throws java.lang.Exception;
  Code:
     0: aconst_null   
     1: astore_1      
     2: aconst_null   
     3: astore_2      
     4: new           #2                  // class java/io/FileInputStream
     7: dup           
     8: ldc           #3                  // String 1
    10: invokespecial #4                  // Method java/io/FileInputStream."<init>":(Ljava/lang/String;)V
    13: astore_1      
    14: new           #2                  // class java/io/FileInputStream
    17: dup           
    18: ldc           #5                  // String 2
    20: invokespecial #4                  // Method java/io/FileInputStream."<init>":(Ljava/lang/String;)V
    23: astore_2      
    24: new           #6                  // class Exceptions$CompoundResource
    27: dup           
    28: aload_0       
    29: aload_1       
    30: aload_2       
    31: invokespecial #7                  // Method Exceptions$CompoundResource."<init>":(LExceptions;Ljava/io/Closeable;Ljava/io/Closeable;)V
    34: areturn       
    35: astore_3      
    36: aload_2       
    37: ifnull        44
    40: aload_2       
    41: invokevirtual #9                  // Method java/io/InputStream.close:()V
    44: aload_1       
    45: ifnull        52
    48: aload_1       
    49: invokevirtual #9                  // Method java/io/InputStream.close:()V
    52: aload_3       
    53: athrow        
  Exception table:
     from    to  target type
         4    34    35   Class java/lang/Throwable

Next is some code for check-for-failure-in-finally-and-cleanup with otherwise the same semantics:

public CompoundResource finallyHack() throws Exception {
    InputStream stream1 = null;
    InputStream stream2 = null;
    boolean success = false;
    try {
        stream1 = new FileInputStream("1");
        stream2 = new FileInputStream("2");
        success = true;
        return new CompoundResource(stream1, stream2);
    } finally {
        if (!success) {
            if (stream2 != null) {
                stream2.close();
            }
            if (stream1 != null) {
                stream1.close();
            }
        }
    }
}

That compiles to the following:

public Exceptions$CompoundResource finallyHack() throws java.lang.Exception;
  Code:
     0: aconst_null   
     1: astore_1      
     2: aconst_null   
     3: astore_2      
     4: iconst_0      
     5: istore_3      
     6: new           #2                  // class java/io/FileInputStream
     9: dup           
    10: ldc           #3                  // String 1
    12: invokespecial #4                  // Method java/io/FileInputStream."<init>":(Ljava/lang/String;)V
    15: astore_1      
    16: new           #2                  // class java/io/FileInputStream
    19: dup           
    20: ldc           #5                  // String 2
    22: invokespecial #4                  // Method java/io/FileInputStream."<init>":(Ljava/lang/String;)V
    25: astore_2      
    26: iconst_1      
    27: istore_3      
    28: new           #6                  // class Exceptions$CompoundResource
    31: dup           
    32: aload_0       
    33: aload_1       
    34: aload_2       
    35: invokespecial #7                  // Method Exceptions$CompoundResource."<init>":(LExceptions;Ljava/io/Closeable;Ljava/io/Closeable;)V
    38: astore        4
    40: iload_3       
    41: ifne          60
    44: aload_2       
    45: ifnull        52
    48: aload_2       
    49: invokevirtual #9                  // Method java/io/InputStream.close:()V
    52: aload_1       
    53: ifnull        60
    56: aload_1       
    57: invokevirtual #9                  // Method java/io/InputStream.close:()V
    60: aload         4
    62: areturn       
    63: astore        5
    65: iload_3       
    66: ifne          85
    69: aload_2       
    70: ifnull        77
    73: aload_2       
    74: invokevirtual #9                  // Method java/io/InputStream.close:()V
    77: aload_1       
    78: ifnull        85
    81: aload_1       
    82: invokevirtual #9                  // Method java/io/InputStream.close:()V
    85: aload         5
    87: athrow        
  Exception table:
     from    to  target type
         6    40    63   any
        63    65    63   any

Looking carefully at what is going on here, it seems to be generating the same bytecode as if you had duplicated the entire finally block both at the point of return and inside the catch block. In other words, it is as if you had written this:

public CompoundResource finallyHack() throws Exception {
    InputStream stream1 = null;
    InputStream stream2 = null;
    boolean success = false;
    try {
        stream1 = new FileInputStream("1");
        stream2 = new FileInputStream("2");
        success = true;
        CompoundResource result = new CompoundResource(stream1, stream2);
        if (!success) {
            if (stream2 != null) {
                stream2.close();
            }
            if (stream1 != null) {
                stream1.close();
            }
        }
        return result;
    } catch (any t) {    // just invented this syntax, this won't compile
        if (!success) {
            if (stream2 != null) {
                stream2.close();
            }
            if (stream1 != null) {
                stream1.close();
            }
        }
        throw t;
    }
}

If someone actually wrote that code, you would laugh at them. In the success branch, success is always true, so there is a large chunk of code which never runs, so you're generating bytecode which is never executed, serving only to bloat up your class file. In the exception branch, success is always false, so you're executing an unnecessary check on the value before doing the cleanup which you know has to happen, which again, just adds to the size of the class file.

The most important thing to notice is:

Both the catch (Throwable) and the finally solution actually catch all exceptions.

So as far as answering the question, "Is it OK to catch Throwable for performing cleanup?"...

I am still not sure, but I know that if it's not OK to catch Throwable for it, it's not OK to use finally for it either. And if finally is not OK either, what is left?

Answer 4

Catching Throwable and finally are not interchangeable.

• Code in a finally clause will be executed on exit from the block regardless of the reason for exit. It will be executed if no exception is thrown. It is therefore the appropriate location for cleanup code that must always be executed.

• The catch Throwable code will be executed only if an exception is thrown.