tags:

views:

2467

answers:

11

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!")

A: 

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

Bill James
I agree, I don't see why this would be downvoted.
BobbyShaftoe
not worth a negative vote, but its not an anti-pattern. a finally block allows a certain piece of code to always run, like closing resources, or releasing locks, which may not throw any exceptions.
Chii
My point was that the missing Catch was the anti-pattern in the above, regardless of whether a Finally was there or not. Gobbling exceptions is what caused the debug issue, exacerbated by exception-possible code in the Finally.
Bill James
As far as the down-votes go. A lot of people just pile them on once they see the first one. They feel they should downvote SOMETIMES, and don't want to make the decision themselves.
Bill James
+21  A: 

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.

dsimcha
But in this case, the root cause exception was not allowed to bubble up, because the finally stepped on it (thus causing it to be gobbled.)
Jared
Ok, but that's a corner case. You're asking if try-finally w/o catch is an anti-pattern in general.
dsimcha
Agreed, dsimcha. I'm hoping that there is consensus that in general, this is not an anti-pattern. Perhaps an anti-pattern is, "Finally blocks throwing exceptions"?
Jared
Great answer, just wanted to add that this is usually called "ducking" the exception.
OscarMk
+7  A: 

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.

Bryan Kyle
If you remove the catch (nested in the finally) from this example, then if the try throws an IOException, and the close() does as well, you can't tell from the stack trace that the root cause of the problem is the original IOException. Is this just one of those,"Yep, this will always be hard" cases?
Jared
@Jared: Yep, this will always be hard. :)
Bryan Kyle
FYI - when you removed the catch from the finally block, you basically said that you didn't care about where the exception came from...
Kevin Day
+17  A: 

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

Logan Capaldo
+1  A: 

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

Julien Grenier
Doesn't that suffer from the same problem of an exception in the finally gobbling an exception in the try?
Jared
+1  A: 
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.

TofuBeer
Yep...agreed. This continues to point to the real anti-pattern being a finally that throws an exception.
Jared
A: 

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.

Jason
+8  A: 

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.

OscarRyz
+3  A: 

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.
Ravi Wallau
+1  A: 

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.

Tom Hawtin - tackline
A: 

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

Mark