Finally block when no exceptions thrown

Question

I'm doing some cleanup operations that could throw an exception, and I want to implement the logic of a finally block, but only if no exceptions are thrown:

The initial implementation is this:

acquireResource();
try {
    // stuff
}
catch (Exception e) {
    cleanupFromException(e);
    throw e;
}
finally {
    cleanupTidy();
}

However, if an exception is thrown, this will call cleanupFromException, then cleanupTidy (which is what I don't want).

So my next implementation is this:

acquireResource();
try {
    // stuff
    cleanupTidy();
}
catch (Exception e) {
    cleanupFromException(e);
    throw e;
}

However, this will not call cleanupTidy if the code in the try block does any jumps - return, continue, break, etc - to the outside of the try block.

The best I can come up with is this:

acquireResource();
boolean exceptionThrown = false;
try {
    // stuff
}
catch (Exception e) {
    exceptionThrown = true;
    cleanupFromException(e);
    throw e;
}
finally {
    if (!exceptionThrown)
        cleanupTidy();
}

But this smells a bit whiffy, and is a lot of boilerplate. Is there any better way of having a finally that only executes if an exception is not thrown (in Java)? Or a way of wrapping this in a simpler API?

Would it be possible to make one cleanup method with an optional parameter? — Florian, Jul 04 '16 at 11:28
The important thing is that I want that cleanup method to execute once only, irregardless of whether an exception was thrown or not, or how the try block exited. — thecoop, Jul 04 '16 at 11:30
"*I want that cleanup method to execute once only*" -- then the cleanup methods should ensure that. I would not make it a responsibility of the caller: (1) it's to error prone (that's why we have this discussion at all) and (2) it makes the code too verbose with stuff that belongs onto another abstraction level. — JensG, Jul 05 '16 at 09:28
_"However, this will not call cleanupTidy if the code in the try block does any jumps to the outside of the try block"_ -- don't do that then? — RemcoGerlich, Jul 05 '16 at 10:17

score 3 · Answer 1 · answered Jul 05 '16 at 08:07

The simple solution here is to restructure your code. Rather than:

acquireResource();
try {
    // stuff
    cleanupTidy();
}
catch (Exception e) {
    cleanupFromException(e);
    throw e;
}

Move stuff to another method:

acquireResource();
try {
     doStuff();
     cleanupTidy();
}
catch (Exception e) {
    cleanupFromException(e);
    throw e;
}

That way, the contents of doStuff can break, continue or return as much as it likes, and cleanupTidy() will always be called unless an exception occurs.

Also, I'd advise against catching Exception. You should be more specific over which exceptions you catch.

score 2 · Answer 2 · answered Jul 04 '16 at 11:31

You are right to be concerned. Your code implies that there is something about an exception not having been thrown that makes you want to call cleanupTidy(). That is not true, but someone reading the code in future (even you reading the code in 20 years' time) may think that it is.

The following code executes identically to yours, but does not tell lies:

acquireResource();
boolean cleanupNeeded = true;
try {
    // stuff
}
catch (Exception e) {
    cleanupNeeded=false;
    cleanupFromException(e);
    throw e;
}
finally {
    if (cleanupNeeded)
      cleanupTidy();
}

If there were somewhere to store the cleanupNeeded flag inside something that was visible to cleanupFromException and cleanupTidy, then I would be inclined to put all the cleanupNeeded handling inside those functions. Indeed, I would go further, since cleanupNeeded is only a shorthand for "there exist things that need cleaning up". I would consider each of those things, and find a way of deciding within the cleanup function whether that thing needs cleaning up. You may implicitly have to do this anyway, since depending how far through "stuff" you have got, there will be different things that do or do not need to be cleaned up.

score 1 · Answer 3 · answered Jul 04 '16 at 11:38

I want to implement the logic of a finally block, but only if no exceptions are thrown

A block of code that's run if no Exceptions are thrown?

That's just a block of code after everything else that might throw an Exception.

However, this will not call cleanupTidy if the code in the try block does any jumps - return, continue, break, etc - to the outside of the try block.

So you need a "finally" block to be run after your "normal" processing but before any Exception handling? Sounds like you need a nested try .. finally block:

acquireResource();
try {
    try {
        // stuff
    } 
    finally { 
        cleanupTidy();
    }
}
catch (Exception e) {
    cleanupFromException(e);
    throw e;
}

Also; why are you catching Exception?

To me, catching the fundamental base class for all Exception Types raises a [big] Red Flag. Can you be any more specific in the [type of] Exception that you're catching? The fact that you're passing the Exception as an argument to a "cleanup" routine makes me think that that clean-up routine is likely to be doing too much and will be too far removed from the Scene of the Crime. You might be better off with clean-up code specific to each Type of Exception that you catch:

try { 
    // stuff 
} 
catch( IOException e ) { 
    // I/O cleanup 
}
catch( NullReferenceException e ) { 
    // Null reference cleanup 
} 
catch( Exception e ) {
    // Any other business 
}

the nested try/catch won't quite work: if `stuff` throws, first `cleanupTidy()` is run, then additionally `cleanupFromException()`. — amon, Jul 04 '16 at 14:39

thecoop · Answer 4 · 2016-07-05T09:12:54.213

1

I've found a (somewhat icky) solution with less boilerplate. I was passing in my exception so I could add any exceptions thrown while closing to the suppressed exceptions of the original exception. However, try-with-resources does this for me:

acquireResource();
try (Closeable c = this::cleanup) {
    // stuff
}

This is not a good use of try-with-resources, but it does solve my problem - the close is only called once, and any exceptions thrown will be propagated properly, with no exception hiding.

edited Jul 05 '16 at 09:12

answered Jul 04 '16 at 11:52

thecoop

491
1
5
13

Are you sure this does what you asked for? Try-with-resources should call close on the resource in all cases, it's equivalent to putting an explicit call to close in a finally block. – Harry Harrison Jul 05 '16 at 07:17

score 0 · Answer 5 · answered Jul 05 '16 at 10:02

A "finally" block contains that is executed, no matter what. Especially it is intended to be executed if an exception is thrown. If you put code into a finally block and then prevent it from being executed, for example by setting cleanupNeeded = false, then you are not using it as intended. The code above could be changed to

acquireResource();
try {
    // stuff
}
catch (Exception e) {
    cleanupFromException(e);
    throw e;
}

cleanupTidy();

The OP basically said: I'm using "finally", but it doesn't do what I want. The simple answer is: Don't use "finally".

Finally block when no exceptions thrown

5 Answers5