13

In Rational Application Developer(RAD based on eclipse), under software analyzer, I see a code review comment (under Performance =>Memory section) saying "Avoid the throw statement inside of finally".

How does defining throw inside a finally block affect performance?

enter image description here

Here is code snippet, we have already suggested to change code to log exception trace and do not throw exception,

     } finally {
        if (bufferedReader != null) {
            try {
                bufferedReader.close();
            } catch (final IOException ex) {
                throw ex;
            }
        }
    }

I was just wondering how this could affect memory and performance?

Stuck in Java
  • 202
  • 2
  • 12
  • 13
    **Why on earth would you ever throw an exception in a `finally` block??** Don't do this. Ever. – Boris the Spider Jul 30 '15 at 13:52
  • 4
    If you have two objects that need cleanup, `A` and `B`, but cleanup of `A` already throws an exception, the missing cleanup of `B` might create a memory leak... But that's just a guess. It's not a good idea to throw exceptions inside the finally block but memory issues would be not the top one reason for me. – Florian Schaetz Jul 30 '15 at 13:53
  • 1
    http://stackoverflow.com/questions/481446/throws-exception-in-finally-blocks – brso05 Jul 30 '15 at 13:54
  • http://www.javamex.com/tutorials/exceptions/exceptions_finally.shtml – brso05 Jul 30 '15 at 13:55
  • http://programmers.stackexchange.com/questions/188858/throwing-an-exception-inside-finally – brso05 Jul 30 '15 at 13:57
  • 4
    @BoristheSpider The plainest of plain `finally` blocks, `{ stream.close(); }` involves a method call that declares to throw `IOException`. So it would seem that _everyone_ is doing it. – Marko Topolnik Jul 30 '15 at 14:01
  • 4
    `catch (final IOException ex) { throw ex; }` -- this code is meaningless. Delete it and you have exact same behavior. – Marko Topolnik Jul 30 '15 at 14:03
  • @FlorianSchaetz thank you for the explanation , this helps to answer my question . thank you. – Stuck in Java Jul 30 '15 at 14:08
  • What "code review" tool is this? A plug-in, or something built into Mars' JDT (which I haven't tried yet)? – erickson Jul 30 '15 at 17:59
  • @erickson we are using RAD , it is software analyzer , right click on files or project , you will see software analyzer then created new configuration select code review rules and run . – Stuck in Java Jul 30 '15 at 19:54
  • @StuckinJava Can you try switching that view to a table? Or hover over the error in the tree view? Often static analysis tools have a longer descriptive field for each issue they detect that might shed some light. – erickson Jul 30 '15 at 20:11
  • @erickson tried to hover over, but tool is not giving any more details. also there is no table view – Stuck in Java Jul 31 '15 at 13:51

6 Answers6

6

An exception thrown from the finally block will replace any exception that was thrown from the try, and information about the real problem is likely to be lost.

Since the try-finally block is allowed to throw an IOException in this case, here's a better way to write it:

try (BufferedReader bufferedReader = Files.newBufferedReader(Paths.get("file.txt"))) {
  /* Work with `bufferedReader` */
}

This automatically closes the reader when the block exits, and handles any resulting exceptions nicely, even if the code inside the try block throws its own exception first, using the "suppression" mechanism of Throwable.

If the try block completes without exception, the result will be the result of closing the resource (exception or not). If the try block throws an exception, that will be the exceptional result. But if the close() method raises an exception too, it will be added to the try block's exception as a "suppressed" exception. You can interrogate it programmatically, and when the stack trace is printed, the suppressed exception will be displayed, much like the "caused by" exceptions you may be more familiar with.

And, you can try-with-multiple resources; these will all be closed, and multiple closure exceptions can be suppressed.

This assumes you're using file I/O, but the same "try-with-resources" structure will work on anything that implements AutoCloseable (streams, SQL objects, etc.).

erickson
  • 265,237
  • 58
  • 395
  • 493
  • @MarkoTopolnik You're right. I think I've got the right answer, and beefed up my good example (which I hope wasn't senseless even before edits). – erickson Jul 30 '15 at 16:55
  • 2
    What remains unanswered is, where's the impact on performance. I actually believe the warning is simply misclassisfied. – Marko Topolnik Jul 30 '15 at 17:48
  • This answer makes sense to me after the edit. I think it is an elegant way of handling it. @MarkoTopolnik. I answered the performance question in my answer. I think the performance hit is likely to be minimal, but anecdotally, essentially rethrowing an exception that has already been thrown is less efficient than handling the error when it is first thrown. The other issue is that when you use a throw in a finally block, it seems to me that you are risking not finishing up necessary cleanup. – stubsthewizard Jul 30 '15 at 18:59
  • @stubsthewizard Yes, all that is said is correct, just it doesn't have overlap with the actual question. Handling the exception within the method where it is thrown allows certain JIT optimizations to be applied, but that's again irrelevant to this as it has nothing to do with throwing from `finally`. Your final point has nothing to do with performance. And on a general note, exception handling hasn't been a peformance bottleneck in any application I ever saw. What kind of code throws and catches in a hot loop, millions of times per second? If it does, then _that_ is the problem. – Marko Topolnik Jul 30 '15 at 19:10
  • @MarkoTopolnik. I am not sure I am putting this correctly. It seems to me that the close() method is already throwing the error, so throwing it again in the finally block is an unnecessary bit of processing, that degrades performance, however minimally. In other words, rather than throwing twice and handling once, to me, it would be slightly more efficient to throw once and handle once. I am not not disagreeing with you, and am just asking for clarification from an established expert with all the respect I can muster. – stubsthewizard Jul 30 '15 at 19:21
  • I suppose that one way to test it would be to write code that handles the exception within the finally block, and time it against code that rethrows the exception to something else to handle. My suspicion is that the performance hit will be so minimal as to not effectively matter in a real application. Whether or not it affects performance much, I would still not throw an exception inside a finally block. – stubsthewizard Jul 30 '15 at 19:26
  • 1
    @stubsthewizard I think there may be a misunderstanding stemming from terminology here: _catching_ the exception is not the same as _handling_ it. OP's code does no handling, it just catches and immediately rethrows. That idiom is strictly redundant and bad practice as such, but it has very little to do with performance and the little bit that has to do with performance, doesn't have anything to do with the `finally` block. This question is not about rethrowing, it's about a supposed effect of throwing an exception from the `finally` block on performance. – Marko Topolnik Jul 30 '15 at 19:41
  • @MarkoTopolnik. Thank you for your input. I am on the same page with you, now. I was focused on the rethrowing itself, but not why a finally block would have anything to do with the efficiency of rethrowing. I am agreed that doing it within a finally block does not make it any more of a performance hit than anywhere else. – stubsthewizard Jul 30 '15 at 21:15
3

This is not a performance issue. It is a correctness issue. (Marko Topolnik's comment that the warning is misclassified seems correct to me, the only way I can see a performance angle to this is that if an exception thrown in the try block gets masked, the effort spent creating it and its stack trace was wasted. But this is a long way from being the biggest problem.)

Two reasons not to throw an exception in a finally block:

  • Letting an exception get thrown from the finally block can mask any exception thrown in the try block, so you lose the original, useful exception, leaving no clue in the logs as to what actually went wrong.

  • When your normal flow of control gets interrupted for an exception thrown on close, you may be letting some transitory I/O glitch (that you don't have any control over and which didn't affect your business logic) prevent some piece of useful work from getting accomplished (for instance, it might cause the current transaction to be rolled back). This may depend on what kind of resource is involved; maybe there is some case where you really may want to fail the whole thing if the close doesn't happen cleanly, but for a lot of common cases (like JDBC) there is no good reason to care.

Using try-with-resources successfully excludes the possibility of exception-masking. However, if the try logic completes without an exception, it lets anything thrown on close get propagated. Since it's a language addition Oracle has to take the most conservative approach, just be aware what it's doing when you use it.

Nathan Hughes
  • 94,330
  • 19
  • 181
  • 276
1

Ideal solution:

try (BufferedReader bufferedReader = ...) {
  //do stuff
}

But perhaps you're in java 1.6:

BufferedReader bufferedReader = null;
try{
  bufferedReader = ...;
  //do stuff
} finally {
  if (bufferedReader != null) {
    try {
      bufferedReader.close();
    } catch (final IOException ex) {
      logger.error("Problem while cleaning up.", ex);
    }
  }
}
Andreas
  • 4,937
  • 2
  • 25
  • 35
0

In finally block you can call another method passing object B to clean up. And you can have try catch finally blocks in that method.

Its good if you have clean up methods for each object A & B separately, in which you handle try catch finally blocks. You can call both the methods for cleaning up objects. Exceptions from each object clean up are independently handled.

Nandan P
  • 106
  • 4
0

In the code snippet given, there is no point to the throw statement. The exception would have already been thrown by the bufferedReader.close() method. The catch block should just handle it, rather than throwing another exception. In fact, while catching a thrown exception is certainly valid in a finally block, I cannot really think of a valid reason to throw an exception in a finally block.

As to performance degradation, from an anecdotal standpoint, rethrowing a caught exception is clearly less efficient than just handling it.

As to a specific instance where this would be detrimental, something like this comes to mind off the top of my head. If you had another method performing cleanup in your finally block, such as FileOutputStream.close(),and the first one threw an error, the second would never run, as the throw statement ends the processing of the block. At that point you would be leaking resources.

So, to sum it up, try/catch are fine in a finally block, but the use of the throw statement should be avoided for the sake of efficiency as well as unintended consequences (memory leak).

stubsthewizard
  • 352
  • 2
  • 12
-1

Finally is used for after an exception is thrown/caught. So by design, there is no need to throw an exception in finally. This should only be done in your try block.

This might help you to learn more about try-catch-finally exception handling in Java

Paradox
  • 4,602
  • 12
  • 44
  • 88
  • 3
    The finally block is **always** executed, even if the try block did **not** throw an exception. – hzpz Jul 30 '15 at 18:53