33

Static code analyzers like Fortify "complain" when an exception might be thrown inside a finally block, saying that Using a throw statement inside a finally block breaks the logical progression through the try-catch-finally. Normally I agree with this. But recently I've come across this code:

SomeFileWriter writer = null; 
try { 
     //init the writer
     //write into the file
} catch (...) {
     //exception handling
} finally {
     if (writer!= null) writer.close();  
}

Now if the writer cannot be closed properly the writer.close() method will throw an exception. An exception should be thrown because (most probably) the file wasn't saved after writing.

I could declare an extra variable, set it if there was an error closing the writer and throw an exception after the finally block. But this code works fine and I'm not sure whether to change it or not.

What are the drawbacks of throwing an exception inside the finally block?

superM
  • 7,363
  • 4
  • 29
  • 38
  • 4
    If this is Java, and you can use Java 7, check out if ARM blocks can solve your problem. – Landei Mar 01 '13 at 12:11
  • @Landei, this solves it, but unfortunately we're not using Java 7. – superM Mar 01 '13 at 12:15
  • I would say that the code you have shown isn't "Using a throw statement inside a finally block" and as such the logical progression is just fine. – Mike Mar 01 '13 at 13:08
  • @Mike, I've used the standard summary that Fortify shows, but directly or indirectly there is an exception thrown inside finally. – superM Mar 01 '13 at 13:10
  • Unfortunately, try-with-resources block is also detected by Fortify as exception thrown inside finally.. it's too smart, damn.. still not sure how to overcome it, it seemed the reason for try-with-resources was to assure closing the resources finally, and now each such statement is reported by Fortify as a security threat.. – mmona Nov 04 '15 at 15:45
  • If Fortify complains about try-with-resources with a single resource variable, then it has a bug. It has to assume that the close method of the resource is implemented to correctly clean up the resource even in the presence of some failure, or else it's impossible to write sane code. – Sebastian Redl Jan 05 '16 at 12:54

4 Answers4

22

Basically, finally clauses are there to ensure proper release of a resource. However, if an exception is thrown inside the finally block, that guarantee goes away. Worse, if your main block of code throws an exception, the exception raised in the finally block will hide it. It will look like the error was caused by the call to close, not for the real reason.

Some people follow a nasty pattern of nested exception handlers, swallowing any exceptions thrown in the finally block.

SomeFileWriter writer = null; 
try { 
     //init the writer
     //write into the file
} finally {
    if (writer!= null) {
        try {
            writer.close();
        } catch (...) {
            // swallow
        }
    }
}

In older versions of Java, you can "simplify" this code by wrapping resources in classes that do this "safe" clean up for you. A good friend of mine creates a list of anonymous types, each that provide the logic for cleaning up their resources. Then his code simply loops over the list and calls the dispose method within the finally block.

Koray Tugay
  • 1,565
  • 3
  • 12
  • 18
Travis Parks
  • 2,495
  • 1
  • 19
  • 23
  • 1
    +1 Though I'm among those who use that nasty code. But to my justification I will say that I don't do this always, only when the exception isn't critical. – superM Mar 01 '13 at 13:43
  • 2
    I find myself doing it as well. Some times creating any entire resource manager (anonymous or not) detracts from the purpose of the code. – Travis Parks Mar 01 '13 at 13:45
  • +1 from me too; you basically said exactly what I was going to but better. Worth noting is that some of the Stream implementations in Java can't actually throw Exceptions on close(), but the interface declares it because some of them do. So, in some circumstances you might be adding a catch block which will never actually be needed. – vaughandroid Mar 01 '13 at 13:50
  • 1
    What's so nasty about using a try/catch block within a finally block? I would rather do that than bloat all my code by having to wrap all my connections and streams, etc. just so they can be closed quietly - which, in itself, introduces a new problem of not knowing if closing down a connection or stream (or whatever) causes an exception - which is something you may actually like to know about or do something about... – ban-geoengineering May 28 '14 at 17:18
11

What Travis Parks said is true that exceptions in the finally block will consume any return values or exceptions from the try...catch blocks.

If you're using Java 7, though, the problem can be solved by using a try-with-resources block. According to the docs, as long as your resource implements java.lang.AutoCloseable (most library writers/readers do now), the try-with-resources block will close it for you. The additional benefit here is that any exception that occurs while closing it will be suppressed, allowing the original return value or exception to pass up.

From

FileWriter writer = null;
try {
  writer = new FileWriter("myFile.txt");
  writer.write("hello");
} catch(...) {
  // return/throw new exception
} finally {
  writer.close(); // an exception would consume the catch block's return/exception
}

To

try (FileWriter writer = new FileWriter("myFile.txt")) {
  writer.write("hello");
} catch(...) {
  // return/throw new exception, always gets returned even if writer fails to close
}

http://docs.oracle.com/javase/tutorial/essential/exceptions/tryResourceClose.html

clintonmonk
  • 261
  • 1
  • 6
  • 1
    This is helpful sometimes. But in case when I actually need to throw an exception when the file cannot be closed, this approach hides the problems. – superM Mar 01 '13 at 14:57
  • 1
    @superM Actually, try-with-resources doesn't hide an exception from `close()`. "any exception that occurs while closing it will be suppressed, allowing the original return value or exception to pass up" - **this is simply not true**. An exception from `close()` method will be suppressed only when another exception will be thrown from try/catch block. So, if the `try` block doesn't throw any exception, the exception from `close()` method will be thrown (and return value will not be returned). It even can be catched from the current `catch` block. – Ruslan Stelmachenko Mar 14 '18 at 05:37
3

This is going to be more of a conceptual answer to why these warnings might exist even with try-with-resources approaches. It's also unfortunately not the easy kind of solution you might be hoping to achieve.

Error Recovery Cannot Fail

finally models a post-transaction control flow that is executed regardless of whether a transaction succeeds or fails.

In the fail case, finally captures logic that is executed in the middle of recovering from an error, before it has been fully recovered (before we reach our catch destination).

Imagine the conceptual problem it presents to encounter an error in the middle of recovering from an error.

Imagine a database server where we're trying to commit a transaction, and it fails halfway through (say the server ran out of memory in the middle). Now the server wants to roll back the transaction to a point as though nothing happened. Yet imagine it encounters yet another error in the process of rolling back. Now we end up having a half-committed transaction to the database -- the atomicity and the indivisible nature of the transaction is now broken, and the integrity of the database will now be compromised.

This conceptual problem exists in any language that deals with errors whether it's C with manual error code propagation, or C++ with exceptions and destructors, or Java with exceptions and finally.

finally cannot fail in languages that provide it in the same way destructors cannot fail in C++ in the process of encountering exceptions.

The only way to avoid this conceptual and difficult problem is to make sure that the process of rolling back transactions and releasing resources in the middle cannot possibly encounter a recursive exception/error.

So the only safe design here is a design where writer.close() cannot possibly fail. There are usually ways in the design to avoid scenarios where such things can fail in the middle of recovery, making it impossible.

It's unfortunately the only way -- error recovery cannot fail. The easiest way to ensure this is to make those kinds of "resource release" and "reverse side effects" functions incapable of failing. It's not easy -- proper error recovery is hard and also unfortunately hard to test. But the way to achieve it is to make sure that any functions that "destroy", "close", "shutdown", "roll back", etc. cannot possibly encounter an external error in the process, since such functions will often need to be called in the middle of recovering from an existing error.

Example: Logging

Let's say you want to log stuff inside a finally block. This is often going to be a huge problem unless logging cannot fail. Logging almost certainly can fail, since it might want to append more data to file, and that can easily find many reasons to fail.

So the solution here is to make it so any logging function used in finally blocks cannot throw to the caller (it might be able to fail, but it won't throw). How might we do that? If your language allows throwing within the context of finally provided that there's a nested try/catch block, that would be one way to avoid throwing to the caller by swallowing exceptions and turning them into error codes, e.g. Maybe logging can be done in a separate process or thread which can fail separately and outside of an existing error-recovery stack unwind. As long as you can communicate with that process without the possibility of running into an error, that would also be exception-safe, since the safety issue only exists in this scenario if we're recursively throwing from within the same thread.

In this case, we can get away with logging failure provided that it does not throw since failing to log and doing nothing isn't the end of the world (it's not leaking any resources or failing to roll back side effects, e.g.).

Anyway, I'm sure you can already begin to imagine how incredibly difficult it is to truly make a software exception-safe. It might not be necessary to seek this to the fullest degree in all but the most mission-critical software. But it's worth taking note of how to truly achieve exception-safety, since even very general-purpose library authors often fumble here and wreck the whole exception-safety of your application using the library.

SomeFileWriter

If SomeFileWriter can throw inside close, then I'd say it's generally incompatible with exception-handling, unless you never try to close it in a context that involves recovering from an existing exception. If the code for it is outside of your control, we might be SOL but it would be worth notifying the authors of this blatant exception-safety issue. If it's within your control, my main recommendation is to make sure that closing it cannot possibly fail by whatever means necessary.

Imagine if an operating system could actually fail to close a file. Now any program that tries to close a file on shut down will fail to shut down. What are we supposed to do now, just keep the application open and in limbo (probably no), just leak the file resource and ignore the problem (maybe okay if it's not so critical)? The safest design: make it so it's impossible to fail to close a file.

1

I think this is something that you will need to address on a case by case basis. In some cases what the analyser is saying is correct in that the code you have is not that great and needs a rethink. But there may be other cases when throwing or even re-throwing might be the best thing. It's not something you can mandate.

drekka
  • 1,269
  • 1
  • 9
  • 20