Is it safe to nest try/finally clauses like this?

Question

As this is meant as a somewhat academic question regarding the behaviour of the try/finally clause, I tried to use an example that is very generic. Is there any danger in nesting a try/finally clause like this?

openDatabaseConnection();
try {
    // Methods unrelated to cursor
    // ...

    String cursor_id = openCursor();
    try {
        useCursor(cursor_id);
    } finally {
        closeCursor(cursor_id);
    }

    // Methods unrelated to cursor
    // ...
} catch (Exception e) {
    genericLogError();
} finally {
    closeDatabaseConnection();
}

Specifically, I'm interested to know whether or not closeCursor() is guaranteed to be called before closeDatabaseConnection(). Is there reason nesting a finally clause like should be considered bad practice?

Copacetic

call both in same finally method – Barleycorn

@divyabharathi: Please explain why you recommend this. – Copacetic

Answer 1

Yes, it is guaranteed.

Say some exception occurred during useCursor(cursor_id), now the inner finally block will be executed. (Since the exception happened in the inner try block). After this genericLogError() will be called (Since an exception occurred in the outer try that included the inner try. And only after it closeDatabasConnection() will be executed (the finally of the outer try). Here is a diagram that might explain it better:

(Exception in the inner try → finally of the inner try ) → this is considered an exception of the outer try → catch of the outer try → finally of the outer try.

You can test it by printing from the blocks.

But why not to do it like this?

try {
  useCursor(cursor_id);
} catch(Exception e) {
  genericLogError();
} finally {
  closeCursor(cursor_id);
  closeDatabaseConnection();
}

Rorie

Suppose I put closeCursor() in the same finally block as closeDatabaseConnection(). If an exception occurs inside the try block but prior to openCursor(), couldn't this result in closeCursor() being called without a valid cursor_id? – Copacetic

In this case it is better to separate them onto two different blocks. Then closeCursor() will not be called without a valid id. – Rorie

I see. I am curious why so many Java developers encourage me to use one finally clause while nesting them achieves the same goal, and with such simplicity... – Copacetic

You can only have one finally clause for each try-catch statement. But if you have more try-catch you can have more finally of course. – Rorie

Sorry, what I meant is, I am often urged to use one "big" try...finally block (as in your answer) instead of using nested try...finally blocks (as in my starting example). – Copacetic

I don't think there is something wrong about using nested try-catch blocks. It depends on what I want to achieve. In your example, I don't see anything wrong, perhaps you could make it more readable, but the logic itself is OK and doesn't suffer from anything. – Rorie

Answer 2

There is nothing wrong with this, assuming you need to nest the try-catch blocks like this in the first place. Maroun's answer would be better for the example you provided. An example where the approach you suggested would be more suitable might be if you have lots of local variables involved in the "Cursor" clean-up:

openDatabaseConnection();
try {
    Cursor cursor = openCursor();
    CursorHelper helper = new CursorHelper(cursor);
    String cursor_id = cursor.getId();
    String something_else = cursor.getSomethingElse();
    try {
        useCursor(cursor_id, something_else, helper);
    } finally {
        closeCursor(cursor_id, something_else, helper);
    }
} catch (Exception e) {
    genericLogError();
} finally {
    closeDatabaseConnection();
}

The parent try block will catch the exception thrown by the nested one, but the nested finally block will be called first. This keeps the scope of the cursor variables encapsulated within the first try/catch block.

If you wanted to pull all this code into one try/catch block you would have to declare some of your variables outside the block, which can start to affect readability:

openDatabaseConnection();
CursorHelper helper = null;
String cursor_id = null;
String something_else = null;
try {
    Cursor cursor = openCursor();
    helper = new CursorHelper(cursor);
    cursor_id = cursor.getId();
    something_else = cursor.getSomethingElse();
    useCursor(cursor_id, something_else, helper);
} catch (Exception e) {
    genericLogError();
} finally {
    if (cursor_id != null) {
        closeCursor(cursor_id, something_else, helper);
    }
    closeDatabaseConnection();
}

Rinse

Answer 3

Yes, closeCursor() is guaranteed to be called before closeDatabaseConnection(), in this case.

You could put both calls in one finally block (move the call to closeCursor() to the same finally block where you call closeDatabaseConnection()), but that would require declaring the cursor in the outer scope, which may not be desirable. For example, you may want to put all the code working with the cursor into a separate method; in this case you would probably want to have the finally block which closes cursor in this new method.

In general, I don't think nesting of finally clauses should be considered a bad practise. One should rather consider the general readability of the code, things like number of lines in one method - perhaps it would be better to split your method into two, in this case there would be two finally blocks, one in each method. Or, if your code is simple and short enough to be in one method you could reorganize the code to have one finally block only.

Bunghole

Answer 4

The execution of the finally clause is guaranteed when control leaves the try block. Here the control leaves the inner try block before the outer block, so the execution order is guaranteed.

There is no reason why this should be considered bad practice, apart from the fact that you are making a big method that could be split in two or more.

Urogenous

Answer 5

Use try/chatch and then finally.
And then your code is fine.
It is good coding practice to close cursor,connection always and here you have to call closeCursor(cursor_id) before calling closeDatabaseConnection() which exactly you have done.
As finally block will always execute although some Exception occurs during execution of respective blocks, so here in your code cursor will close first and then database connection will close so this is perfect.

Please find the modified code snippet for your reference:

 openDatabaseConnection()
    try {
        // Methods unrelated to cursor
        // ...

        String cursor_id = openCursor();
        try {
            useCursor(cursor_id);
        } 
        catch (Exception e) { //Added the catch block.
        genericLogError();
       } 
        finally {
            closeCursor(cursor_id);
        }

        // Methods unrelated to cursor
        // ...
    } catch (Exception e) {
        genericLogError();
    } finally {
        closeDatabaseConnection();
    }

Or you can remove the inner try and can close both in the outer finally itself like:

//outer try block
try{
....
}catch(Exception e){
genericLogError();
}
finally {
         closeCursor(cursor_id);
        closeDatabaseConnection();
    }

This will also do.

Beverage

Why do you suggest copying the catch block into the inner try? If I don't catch the exception there, won't it escape to the outer try anyway? – Copacetic

You have to add catch after try block. try block without catch block is not possible, as try block is used to catch exception only. Got my point? Or you can remove the inner try and can close both in the outer finally itself like: //outer try block try{ .... }catch(Exception e){ genericLogError(); } – Beverage

Answer updated. Please check once. And let me know in case of any concern. – Beverage

But try blocks aren't only used for catching exceptions, they are also used for clean-up code, as in my example. A try...finally construct with no catch clause is syntactically valid, compiles and executes just fine. – Copacetic

Yes. It will not give an error but this is not good coding practice Stephan. – Beverage

Duplicating code is not a good coding practice. Many experienced users in this thread seem to disagree that a catch clause must always be present. – Copacetic

So, its better to use the second approach Stephen. – Beverage

You should not catch an exception if you can't do anything about it, like retrying the operation or communicating the problem to the user, at that level. Most often you want the exception to bubble up to the appropriate layer, which means try-finally without catch should be more common than try-catch. – Urogenous