4

We have a class we've written which opens up a connection to a server. When you're done with it, you need to either tell it to commit if everything was successful, or tell it to rollback if something went wrong. So right now we have a lot of spots in our code that look like this:

OurConnectionClass conn = null;
try {
    conn = OurConnectionClass(parameters);
    // Do some stuff here...
    conn.commit();
} catch (Throwable t) {
    if (conn != null) {
        conn.rollback();
    }
    throw t;
}

If you forget to commit or rollback, there's no immediate issues, but eventually you exhaust a connection pool and then have to figure out where you made a mistake.

I'd like to find a way so OurConnectionClass implements AutoClosable, so I could do somsething like this instead:

try (OurConnectionClass conn = new OurConnectionClass(parameters)) {
    // Do some stuff here...
}

I feel like there has to be a way to do this, but I'm not seeing it. AutoCloseable only calls a close method, with no arguments passed to it. As far as I can see, there's no way to know whether close is being called because the end of the try block was successfully reached or because an exception was thrown.

ArtOfWarfare
  • 20,617
  • 19
  • 137
  • 193
  • Now that I've written this all out, I'm wondering if maybe rather than trying to use `try-with-resources`, I should instead use an anonymous function? I could make `OurConnectionClass` receive it and try to run it, and if it's successful, it runs `commit()` afterwards, and if it fails, it'll run `rollback()` after... I could have this be a static method that returns nothing, rather than returning an instance. That sounds maybe a bit more complicated than I had in mind... I'll wait to see what others think about this before trying it... – ArtOfWarfare Jul 26 '18 at 14:30
  • with `AutoClosable` you meant `AutoCloseable` ? can you specify what interface are you referring to please? – Leviand Jul 26 '18 at 14:33
  • 1
    Note, if the constructor fails and an exception is thrown no close() is called. – Peter Lawrey Jul 26 '18 at 14:33
  • 1
    According to the [JLS](https://docs.oracle.com/javase/specs/jls/se10/html/jls-14.html#jls-14.20.3) you can define a `catch`-block following a try-with-resource-block where you handle an exception. – LuCio Jul 26 '18 at 14:37
  • @Leviand - I think from context (the fact I'm talking about Java's try-with-resource) that should be pretty obvious that I meant the `AutoCloseable` interface from the standard Java library and not something third party. In any event, you're right, I spelled the name wrong in a few spots. I've fixed it. – ArtOfWarfare Jul 26 '18 at 15:47

3 Answers3

3

When executing this snippet.

try (OurConnectionClass conn = new OurConnectionClass(parameters)) {
    // Do some stuff here...
    conn.commit();
}

OurConnectionClass.close() will always be called after an instance is created. So you can just add some logic to check if a commit was made. E.g. with a boolean flag. With that you could then check in the close() method if the connection should be closed peacefully or it should rollback:

public class OurConnectionClass implements AutoCloseable{

    private boolean committed; // initialized to false

    public void commit(){
         // commit
         committed = true;
    }

    public void close() throws Exception{
         if(!committed){
             // rollback
         }
    }
}
Lino
  • 19,604
  • 6
  • 47
  • 65
  • Probably want to make `committed` `volatile`. – Bohemian Jul 26 '18 at 14:38
  • @Bohemian I am not really sure, but is the logic in the `try-with-resources`-finally block executed in a different thread?. Because inside the `twr` block shouldn't be multiple threads accessing `conn` if I understood the question correctly – Lino Jul 26 '18 at 14:46
  • 1
    Yeah... this certainly sounds like an improvement over what we're doing now, but I'd be concerned about wasted time when developers forget to commit and don't know what's happening. I could just make it so that close called without a commit both calls rollback and throws an exception. I've never seen a rollback used when an exception hadn't occurred anyways. I think I'm going to dig into using lambdas for this instead though... – ArtOfWarfare Jul 26 '18 at 15:44
1

I think the semantic you want is that the transaction is rolled back in close, unless the code which uses OurConnectionClass explicit calls OurConnectionClass.commit().

Then you don't have any problem, because your close method, then just need to test if there is an open transaction. And if there is roll it back, and log an error.

MTilsted
  • 5,425
  • 9
  • 44
  • 76
0

Do both!

try (OurConnectionClass conn = new OurConnectionClass(parameters)) {
    // Do some stuff here...
    conn.commit();
} catch (Throwable t) {
    conn.rollback();
    throw t;
}

The closeable is still auto-closed (in the implicit finally block) if stuff explodes.

BTW, it would be better to throw a domain exception:

throw new MyStuffExploded(t);

because re-throwing the connection exception lets implementation details leak out via the method contract, which is a form of coupling, which is bad.

Bohemian
  • 412,405
  • 93
  • 575
  • 722
  • Should a `Throwable t`only be consumed or rather also rethrown as of it's significance? – LuCio Jul 26 '18 at 14:43