Avoid dangling Transactions

[nodermatt]

Affected Areas

DB

Feature Description

Operations that require a transaction but do not actually modify content typically contain the same code to gracefully close the transaction (e.g Crud#getTables ). An automatic way of closing the transaction (similar to Java handles IO Streams) would benefit both programmer and Polypheny.

I have already pitched the idea to @vogti and we decided to open the discussion to the remaining team members. If you have concerns or praises for the suggestion, I would be glad to hear them.

Use Case(s)

Consider the below code listing:

        try {
            transaction.commit();
        } catch ( TransactionException e ) {
            try {
                transaction.rollback();
            } catch ( TransactionException ex ) {
                log.error( "Could not rollback", ex );
            }
        }

This code feels unnatural because the #getTables method has no specialized error handling nor does it need to actually commit anything. Additionally, if there were changes during the execution of the routine, the common flow is to commit the changes at the end of the transaction after all. Including rollback of the transaction as part of error handling. Lastly, the current way of managing transactions is prone to errors (i.e. dangling transactions) because it's possible to omitt the try-commit-catch-rollback in their method. This can cause a resource leak as Polypheny will keep the transactions open indefinetely.

Possible Solutions

Implement the AutoCloseable interface in TransactionImpl to leverage try-with-resources style. One possible implementation is the below excerpt.

@Override
    public void close() throws Exception {
        try {
            commit();
        } catch (TransactionException commitException) {
            log.error("Couldn't commit pending transaction. Attempting to rollback");
            try {
                rollback();
            } catch (TransactionException rollbackException){
                String rollbackError = "Failed to rollback transaction to remediate failed commit.";
                log.error(rollbackError);
                throw new RuntimeException(rollbackError);
            }
        }
    }

Once the try-block has finished, the close() is invoked and attempts to commit the pending changes. If the commit fails (TransactionException!) a rollback is initiated. If this also fails, Polypheny can't do anymore salvaging and throws a RuntimeException.
On the client side the transaction can be used like so:

        try (Transaction transaction = getTransaction()){
            // Some op
            // ...
        }

Arguably, if there is actual cleanup work to do besides rolling back the transaction, this implementation offers no customizing for that. However, this is not the main goal for the proposed solution. It's intended for cases where programmers would simply duplicate code for the transaction handling because a) it doesn't require surgical changes and b) there is no other way to close the transaction besides commit() or rollback.

Possible Alternatives

Another approach could be to enforce commit/rollback in the #close method. First if the transaction is active and then either commit and log or to throw an RuntimeException to make sure the code won't work if transactions aren't properly closed.

@Override
    public void close() throws Exception {
        try {
            if(isActive()) {
                // either 
                throw new RuntimeException("Transaction wasn't closed. Make sure to either commit or rollback the transaction at some point to avoid resource leaks!");
               // or gracefully close and inform user
               log.warn("The transaction wasn't closed by the enclosing try-with block. Polypheny will attempt to commit the changes to avoid a dangling transaction");
               commit();
            }
        }
        // catch omitted
    }

[nodermatt]

@datomo @isabelge Do you have any objections? Otherwise I'll open a PR for this change.

[datomo]

No objections from my side. As it is mostly a refactor with the minor addition of the RuntimeException, which is catched down the road and shown to the user, this should work as proposed.

Edit: While when only observing this part of Polypheny the problem is solved, the added RuntimeException will not close Polypheny and will still produce a dangling transaction, as it is catched later on. But I still like the change of moving the code into the extended TransactionImpl/Transaction.