Is a finally block without a catch block a java anti-pattern?

Question

I just had a pretty painful troubleshooting experience in troubleshooting some code that looked like this:

try {
   doSomeStuff()
   doMore()
} finally {
   doSomeOtherStuff()
}

The problem was difficult to troubleshoot because doSomeStuff() threw an exception, which in turn caused doSomeOtherStuff() to also throw an exception. The second exception (thrown by the finally block) was thrown up to my code, but it did not have a handle on the first exception (thrown from doSomeStuff()), which was the real root-cause of the problem.

If the code had said this instead, the problem would have been readily apparent:

try {
    doSomeStuff()
    doMore()
} catch (Exception e) {
    log.error(e);
} finally {
   doSomeOtherStuff()
}

So, my question is this:

Is a finally block used without any catch block a well-known java anti-pattern? (It certainly seems to be a not-readily-apparent subclass of the obviously well-known anti-pattern "Don't gobble exceptions!")

Answer 1

I'd say a try block without a catch block is an anti-pattern. Saying "Don't have a finally without a catch" is a subset of "Don't have a try without a catch".

Answer 2

In general, no, this is not an anti-pattern. The point of finally blocks is to make sure stuff gets cleaned up whether an exception is thrown or not. The whole point of exception handling is that, if you can't deal with it, you let it bubble up to someone who can, through the relatively clean out-of-band signaling exception handling provides. If you need to make sure stuff gets cleaned up if an exception is thrown, but can't properly handle the exception in the current scope, then this is exactly the correct thing to do. You just might want to be a little more careful about making sure your finally block doesn't throw.

Answer 3

There is absolutely nothing wrong a try with a finally and no catch. Consider the following:

InputStream in = null;
try {
    in = new FileInputStream("file.txt");
    // Do something that causes an IOException to be thrown
} finally {
    if (in != null) {
         try {
             in.close();
         } catch (IOException e) {
             // Nothing we can do.
         }
    }
}

If an exception is thrown and this code doesn't know how to deal with it then the exception should bubble up the call stack to the caller. In this case we still want to clean up the stream so I think it makes perfect sense to have a try block without a catch.

Answer 4

I think the real "anti-pattern" here is doing something in a finally block that can throw, not not having a catch.

Answer 5

I use try/finally in the following form :

try{
   Connection connection = ConnectionManager.openConnection();
   try{
       //work with the connection;
   }finally{
       if(connection != null){
          connection.close();           
       }
   }
}catch(ConnectionException connectionException){
   //handle connection exception;
}

I prefer this to the try/catch/finally (+ nested try/catch in the finally). I think that it is more concise and I don't duplicate the catch(Exception).

Answer 6

try {
    doSomeStuff()
    doMore()
} catch (Exception e) {
    log.error(e);
} finally {
   doSomeOtherStuff()
}

Don't do that either... you just hid more bugs (well not exactly hid them... but made it harder to deal with them. When you catch Exception you are also catching any sort of RuntimeException (like NullPointer and ArrayIndexOutOfBounds).

In general, catch the exceptions you have to catch (checked exceptions) and deal with the others at testing time. RuntimeExceptions are designed to be used for programmer errors - and programmer errors are things that should not happen in a properly debugged program.

Answer 7

I think that try with no catch is anti-pattern. Using try/catch to handle exceptional conditions (file IO errors, socket timeout, etc) is not an anti-pattern.

If you're using try/finally for cleanup, consider a using block instead.

Answer 8

Not at all.

What's wrong is the code inside the finally.

Remember that finally will always get executed, and is just risky ( as you have just witnessed ) to put something that may throw an exception there.

Answer 9

I think it's far from being an anti-pattern and is something I do very frequently when it's critical do deallocate resources obtained during the method execution.

One thing I do when dealing with file handles (for writing) is flushing the stream before closing it using the IOUtils.closeQuietly method, which doesn't throw exceptions:

OutputStream os = null;
OutputStreamWriter wos = null;
try { 
   os = new FileOutputStream(...);
   wos = new OutputStreamWriter(os);
   // Lots of code

   wos.flush();
   os.flush();
finally {
   IOUtils.closeQuietly(wos);
   IOUtils.closeQuietly(os);
}

I like doing it that way for the following reasons:

• It's not completely safe to ignore an exception when closing a file - if there are bytes that were not written to the file yet, then the file may not be in the state the caller would expect;
• So, if an exception is raised during the flush() method, it will be propagated to the caller but I still will make sure all the files are closed. The method IOUtils.closeQuietly(...) is less verbose then the corresponding try ... catch ... ignore me block;
• If using multiple output streams the order for the flush() method is important. The streams that were created by passing other streams as constructors should be flushed first. The same thing is valid for the close() method, but the flush() is more clear in my opinion.

Answer 10

In my opinion, it's more the case that finally with a catch indicate some kind of problem. The resource idiom is very simple:

acquire
try {
    use
} finally {
    release
}

In Java you can have an exception from pretty much anywhere. Often the acquire throws a checked exception, the sensible way to handle that is to put a catch around the how lot. Don't try some hideous null checking.

If you were going to be really anal you should note that there are implied priorities among exceptions. For instance ThreadDeath should clobber all, whether it comes from acquire/use/release. Handling these priorities correctly is unsightly.

Therefore, abstract your resource handling away with the Execute Around idiom.

Answer 11

Try/Finally is a way to properly free resources. The code in the finally block should NEVER throw since it should only act on resources or state that was acquired PRIOR to entry into the try block.

As an aside, I think log4J is almost an anti-pattern.

IF YOU WANT TO INSPECT A RUNNING PROGRAM USE A PROPER INSPECTION TOOL (i.e. a debugger, IDE, or in an extreme sense a byte code weaver but DO NOT PUT LOGGING STATEMENTS IN EVERY FEW LINES!).

In the two examples you present the first one looks correct. The second one includes the logger code and introduces a bug. In the second example you suppress an exception if one is thrown by the first two statements (i.e. you catch it and log it but do not rethrow. This is something I find very common in log4j usage and is a real problem of application design. Basically with your change you make the program fail in an way that would be very hard for the system to handle since you basically march onward as if you never had an exception (sorta like VB basic on error resume next construct).