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?

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();
}

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();
}

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.

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.

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.