2

I just ran into a hidden gem in one of our Java libraries:

for(Widget w : widgets) {
    if(shouldDoStuff()) {
        try{
            // Do stuff to w.
        } catch(Exception e){
            throw new RuntimeException("Couldn't do stuff.");
        } finally{
            // Compiler warning: finally block does not complete normally
            continue;
        }
    }
}

I know that finally trumps everything, but I'm wondering about 2 things:

  1. What happens when the catch clause does execute? Does the exception get thrown or not? What happens first: the thrown exception or the continue statement?
  2. How can I rewrite this to get rid of the warning?

I found this very similar question but the accepted answer just states that the exception will be thrown abruptly, and I'm not sure what that means. Plus it doesn't really help me understand my first question above about the order of events that will transpire.

Community
  • 1
  • 1
AdjustingForInflation
  • 1,571
  • 2
  • 26
  • 50
  • What do you mean by "trumps everything"? – fge Feb 28 '14 at 14:13
  • This is a duplicate of http://stackoverflow.com/questions/5126455/in-java-what-if-both-try-and-catch-throw-same-exception-and-finally-has-a-return which does answer the question nicely (read the explanation). Basically the exception is getting swallowed by the return. Basically rendering the throw useless. – M. Deinum Feb 28 '14 at 14:14
  • Finally block is supposed to do ending things like cleanup tasks. It should not do other things than that. if you remove the continue statement it won't show the warning. – stinepike Feb 28 '14 at 14:15
  • 2. if `continue;` is the only statement in the `finally` block, just move it out of `try/catch` and drop `finally` – fge Feb 28 '14 at 14:15
  • Here's another similar SO question with a bunch of explanations: http://stackoverflow.com/questions/65035/does-finally-always-execute-in-java/65949 – Daniël Knippers Feb 28 '14 at 14:15

3 Answers3

5

"finally" will be executed after your RuntimeException is thrown and before it is processed by another catch upper in the stack.

As your finally just continues, in fact it will do nothing.

The contradiction is between the throw in the catch that will end the loop and the continue.

One approach could be:

boolean exceptionOccured = false;
for(Widget w : widgets) {
    if(shouldDoStuff()) {
        try {
            // Do stuff to w.
        } catch(Exception e){
            exceptionOccured = true;  // do not throw yet.
            e.printStackTrace();
        }
    }
}
if (exceptionOccured) {
  throw new RuntimeException("Couldn't do stuff.");
}

The main concern with this approach is you don't know what went wrong.

Nicolas Defranoux
  • 2,646
  • 1
  • 10
  • 13
  • The continue statement doesn't belong in the `finally` block, which is why the compiler is complaining. (A `return` wouldn't belong there either.) Outside of that, this is the right answer; finally runs before the block exits no matter how it exits. And no, I have no idea what they meant by `abruptly` either. – keshlam Feb 28 '14 at 14:21
  • Thanks @Nicolas Defranoux (+1) - please see my comment underneath GI Joe's answer - I have the same question for you! – AdjustingForInflation Feb 28 '14 at 14:24
  • The continue tries to continue the loop, but as there is a throw in the catch, the finally is executed and then then the exception is thrown higher and the loop is broken anyway. – Nicolas Defranoux Feb 28 '14 at 14:37
  • The question is: what do yo want to do ? If you want to process all the elements in the list even if some exceptions occurs, remove the throw from the catch. You can still store the exception and throw is after the loop ends, though what would you do if there are multiple exceptions ? – Nicolas Defranoux Feb 28 '14 at 14:38
  • Why not remember the actual exception rather than a `boolean`? Then you could re-throw the real exception instead of an unspecific `RuntimeException`. – Holger Feb 28 '14 at 15:32
  • Sure, that will work... but what happens if two exception occurs ? You need a list of exceptions and then you can create a special Exception that handles multiple exceptions so that you can throw them all at once. – Nicolas Defranoux Mar 01 '14 at 12:04
1

The exception is thrown. If ran, you would see:

  1. RuntimeException("Couldn't do stuff.")
  2. Do what's in finally.

If there was an outer try/catch that caught the RuntimeException, then the program could feasibily continue. The continue statement in the finally block is useless. The only time finally won't be called is if you call System.exit() or if the JVM crashes first.

Engineer2021
  • 3,288
  • 6
  • 29
  • 51
  • Thanks @GI Joe (+1) - can you explain why the `continue` statement is **useless**? I think the author of this code wanted to, essentially, fork execution and have the exception rethrown but to still continue processing the list of `Widgets`. – AdjustingForInflation Feb 28 '14 at 14:24
  • @TotesMcGotes: This example you have shows an exception anti-pattern. It's a bad example. Also, the use of `finally` isn't required in JDK7+. But it is typically used for cleaning up resources. – Engineer2021 Feb 28 '14 at 14:32
0

Code of the form

try {
  // non-control-flow statements A
} finally {
  // non-control-flow statements B
}

is compiled as if you had written:

try {
  // non-control-flow statements A
} catch(Throwable t) {
  // non-control-flow statements B
  throw t;
}
// non-control-flow statements B

So that the statements B are executed in either case, exceptional or non-exceptional. But if you insert a control flow statement like continue into the finally block you are turning the hidden re-throw t; statement into dead code.

There is a simple way to achieve the same without a warning. Write an explicit exception handler doing the same but not containing that ineffective re-throw statement. That way you document that swallowing the exception is intentional. Then the warning will disappear. However, swallowing exceptions still remains bad code style.

Holger
  • 285,553
  • 42
  • 434
  • 765