advice on nested Java try/finally code sandwiches

Question

I would like some advice on a technique I bumped onto. It can be easily understood by looking at the code snippets, but I document it somewhat more in the following paragraphs.

---

Using the "Code Sandwich" idiom is commonplace to deal with resource management. Used to C++'s RAII idiom, I switched to Java and found my exception-safe resource management resulting in deeply nested code, in which I have a really hard time getting grip on the regular control flow.

Apparently (java data access: is this good style of java data access code, or is it too much try finally?, Java io ugly try-finally block and many more) I'm not alone.

I tried different solutions to cope with this:

1. maintain the program state explicitly: resource1aquired, fileopened..., and cleanup conditionally: if (resource1acquired) resource1.cleanup()... But I shun duplicating the program state in explicit variables - the runtime knows the state, and I don't want to care for it.

2. wrap every nested block in functions - results in even harder to follow control flow, and makes for really awkward function names: runResource1Acquired( r1 ), runFileOpened( r1, file ), ...

And finally I arrived at an idiom also (conceptually) backed by some research paper on code sandwiches:

---

Instead of this:

// (pseudocode)
try {
   connection = DBusConnection.SessionBus(); // may throw, needs cleanup
   try {
        exported = false;
        connection.export("/MyObject", myObject ); // may throw, needs cleanup
        exported = true;
            //... more try{}finally{} nested blocks
    } finally {
        if( exported ) connection.unExport( "/MyObject" );
    }   
} finally {
   if (connection != null ) connection.disconnect();
}

Using a helper construction, you may arrive at a more linear construct, in which compensation code is right next to the originator.

class Compensation { 
    public void compensate(){};
}
compensations = new Stack<Compensation>();

And the nested code becomes linear:

try {
    connection = DBusConnection.SessionBus(); // may throw, needs cleanup
    compensations.push( new Compensation(){ public void compensate() {
        connection.disconnect();
    });

    connection.export("/MyObject", myObject ); // may throw, needs cleanup
    compensations.push( new Compensation(){ public void compensate() {
        connection.unExport( "/MyObject" );
    });   

    // unfolded try{}finally{} code

} finally {
    while( !compensations.empty() )
        compensations.pop().compensate();
}

I was delighted: no matter how many exceptional paths, the control flow stays linear, and the cleanup code is visually next to the originating code. On top of that, it doesn't need an artificially restricted closeQuietly method, which makes it more flexible (i.e. not only Closeable objects, but also Disconnectable, Rollbackable and whatever others).

But...

I found no mention of this technique elsewhere. So here's the question:

---

Is this technique valid? What bugs do you see in it?

Thanks a lot.

Trickster

You don't seem to be handling the case where an exception could be thrown from within a compensate() method. This would prevent subsequent compensations from running. – Unpleasant

@Kevin: indeed - too much C++ idiom here: destructors must not throw, and I stick to that idiom here. It's up to the compensate implementations to handle exceptions. – Trickster

@Trickster - that seems at odds with you using try-finally at all - you could just adopt that idiom with the code itself, and not bother with a finally block. Besides, many Compensators would naturally throw exceptions (e.g. SQLException for closing a DB resource); every implementation have to catch and swallow all exceptions individually, rather than your finally block handling it consistently in one place. It's not optional for them to do this (a runtime exception would breach their specification, and that could happen any time) so you'd just be encouraging copy-paste. – Muskellunge

this is why I like D's scope(exit) statement – Attenuant

@ratchet freak: which was also mentioned in referred study. – Trickster

Answer 1

It is nice.

No major complaints, minor things off the top of my head:

• a little performance burden
• you need to make some things final for the Compensation to see them. Maybe this prevents some use-cases
• you should catch exceptions during compensate and continue running Compensations no matter what
• (bit far-fetched) you could accidentally empty out the Compensation queue at run-time due to a programming error. OTOH programming errors can happen anyway. And with your Compensation queue, you get "conditional finally blocks".
• don't push this too far. Within a single method it seems okay (but then you probably don't need too many try/finally blocks anyway), but don't pass Compensation queues up and down the call stack.
• it delays compensation to the "outermost finally" which could be a problem for things that need to be cleaned up early
• it only makes sense when you need the try block only for the finally block. If you have a catch block anyway, you might just add the finally right there.

Yate

Thanks. What do you mean by "conditional finally blocks"? – Trickster

Well, you could say if(something) componensation.push(extraCompensation). – Yate

Answer 2

I like the approach, but see a couple of limitations.

The first is that in the original, a throw in an early finally block doesn't affect later blocks. In your demonstration, a throw in the unexport action will stop the disconnect compensation happening.

The second is that it the language is complicated by the ugliness of Java's anonymous classes, including the need to introduce a heap of 'final' variables so they can be seen by the compensators. This is not your fault, but I wonder if the cure is worse than the disease.

But overall, I like the approach, and it is quite cute.

Broadway

Answer 3

The way I see it, what you want are transactions. You compensations are transactions, implemented a bit differently. I assume that you are not working with JPA resources or any other resources that support transactions and rollbacks, because then it would be rather easy to simply use JTA (Java Transaction API). Also, I assume that your resources are not developed by you, because again, you could just have them implement the correct interfaces from JTA and use transactions with them.

So, I like your approach, but what I would do is hide the complexity of popping and compensating from the client. Also, you can pass transactions around transparently then.

Thus (watch out, ugly code ahead):

public class Transaction {
   private Stack<Compensation> compensations = new Stack<Compensation>();

   public Transaction addCompensation(Compensation compensation) {
      this.compensations.add(compensation);
   }

   public void rollback() {
      while(!compensations.empty())
         compensations.pop().compensate();
   }
}

Sigma

... ugly code? I can do uglier:) This is at least a nice way to wrap the technique into a reusable class. – Trickster

the only thing I miss here is try-catch wrapping for compensations.pop().compensate(); to handle exceptions that may occur there as pointed in couple of prior answers – Atingle

Well, I also miss getters and setters, a proper constructor, null parameter checks etc. - pretty ugly to my taste. :) But yeah, gnat, the code to do the rollback should be more elaborate, like checking for invalid or non-functional compensates and the like. What you want to do if a compensation fails is a different story entirely: do you continue or not? Are the compensations depending on each other? Throw an exception (my favourite since your application state is undefined in such a case)? Your requirements probably tell you what to do, so knock yourself out! :) – Sigma

to be clear, my note on wrapping was purely to keep same behavior as in nested try-catches. I didn't mean to do something better than original code, only to read better than it. BTW I like your approach to readability best of all that I saw in answers so far – Atingle

Answer 4

A destructor like device for Java, that will be invoked at the end of the lexical scope, is an interesting topic; it's best addressed at language level, however the language czars don't find it very compelling.

Specifying destruction action immediately after construction action has been discussed (nothing new under the sun). An example is http://projectlombok.org/features/Cleanup.html

Another example, from a private discussion:

{
   FileReader reader = new FileReader(source);
   finally: reader.close(); // any statement

   reader.read();
}

This works by transforming

{
   A
   finally:
      F
   B
}

into

{
   A
   try
   {
      B
   }
   finally
   {
      F
   }
}

If Java 8 adds closure, we could implement this feature succinctly in closure:

auto_scope
#{
    A;
    on_exit #{ F; }
    B;
}

However, with closure, most resource library would provider their own auto cleanup device, clients don't really need to handle it by themselves

File.open(fileName) #{

    read...

}; // auto close

Reckoning

lombok... looks like some std::boost for java :) Thanks for the hint! – Trickster

Answer 5

Do you know you really need this complexity. What happens when you try to unExport something which is not exported?

// one try finally to rule them all.
try {
   connection = DBusConnection.SessionBus(); // may throw, needs cleanup
   connection.export("/MyObject", myObject ); // may throw, needs cleanup
   // more things which could throw an exception

} finally {
   // unwind more things which could have thrown an exception

   try { connection.unExport( "/MyObject" ); } catch(Exception ignored) { }
   if (connection != null ) connection.disconnect();
}

Using helper methods you could do

unExport(connection, "/MyObject");
disconnect(connection);

I would have thought disconnecting would mean you don't need to unExport the resources the connection is using.

Douglass

Then he might run unExport, even though export did not run. Not in the simple example above, but you get the idea. (Could be handled by lots of booleans). – Yate

Indeed: this solution is only useful where the added complexity really adds code-readability. Maybe this example is not exceptional enough :). – Trickster

@Thilo, "Then he might run unExport, even though export did not run." do you know this is even a problem? – Douglass

@xtofl, The Exceptions should be exceptional. If they are a little ugly for the sake of making the core code simpler, its a price worth paying. – Douglass

I think not running unExport makes more sense. It also simplifies the implementation of export and unExport. – Trickster

@xtofl, Do you mean not calling it at all (as I suspect you don't need to) or using a flag which might simplify the library but makes the caller more complex. (IMHO its complexity the library should take from the caller) – Douglass

I mean 'not calling unExport unless you successfully called export'. Your Humble Opinion contradicts mine, apparently :) – Trickster

@xtofl, Its still not clear why you would want to. If you map.put(key, value) you can call map.remove(key); whether the key was added or not. If you have a file you can call delete() whether it exists or not. – Douglass

Yes you can. And if you do, you make your library quite complex. I say it doesn't make sense to close a door you didn't open, so you don't need to implement it. This results in a door that can be closed only if it is open. A 'robust' library may also provide a 'close_if_open' method to the door, but I don't expect my front door to implement this convenience. That's a different discussion, really, but adding this convenience violates the single responsibility principle, and results in often overly complex classes. OTOH, I'm not a library builder, so who am I... – Trickster

But on the other other hand, I think adding 'fool-proofness' to a library is the last thing to do, and comes way after all effort to make it correct. In my particular case (libdbus), I would have loved that. – Trickster

I think you are confusing the action to-close with the desired result to-be-closed. Do you really want the action to fail because you close it when it is already closed, or is it the case you don't want to be exposed to the state of the object and you just want it to ensure it closed. A key principle of Object Orientated programming is encapsulation and not have the caller maintaining a state which the callee has to maintain anyway and leaving you with the task of making sure they're the same. It is possible libdbus was not designed with OOP principles in mind. ;) – Douglass

Answer 6

You should look into try-with-resources, first introduced in Java 7. This should reduce the necessary nesting.

Kilkenny

I did. They restrict the resources to Closeable implementors, which is too restrictive imho. (And I'm not using Java 7 yet, but that's a minor issue:) – Trickster

You could probably file a bug in DBus' bug tracker. Imho there is no reason that this class doesn't implement Closeable. – Kilkenny