[Spanner] TransactionManager.close() not idempotent; API doc example is broken

[archiecobbs]

I am using the new TransactionManager API and am getting mysterious "rollback can only be called if the transaction is in progress" exceptions.

I think I've tracked down the problem...

The API docs example shown with DatabaseClient.transactionManager() is this:

 long singerId = my_singer_id;
 try (TransactionManager manager = dbClient.transactionManager()) {
   TransactionContext txn = manager.begin();
   while (true) {
     String column = "FirstName";
     Struct row = txn.readRow("Singers", Key.of(singerId), Collections.singleton(column));
     String name = row.getString(column);
     txn.buffer(
         Mutation.newUpdateBuilder("Singers").set(column).to(name.toUpperCase()).build());
     try {
       manager.commit();
       break;
     } catch (AbortedException e) {
       Thread.sleep(e.getRetryDelayInMillis() / 1000);
       txn = manager.resetForRetry();
     }
   }
 }

However, this example usage is actually wrong.

The reason is that the implementation returned from DatabaseClient.transactionManager() is actually an AutoClosingTransactionManager (in SessionPool.java), which automatically close()s the transaction if commit() or rollback() is invoked. Sounds like a good idea, right?

The problem is that AutoClosingTransactionManager.close() is not idempotent:

    @Override
    public void close() {
      try {
        delegate.close();
      } finally {
        session.close();
      }
    }

If the transaction is closed twice, then the second time around the session is closed again, even though it has already been returned to the pool and possibly reassigned to another transaction. If so, this in turn causes that other transaction to fail on commit() with an exception "rollback can only be called if the transaction is in progress".

One fix for this is to document that a TransactionManager must only be closed exactly once, and fix your API javadoc example accordingly.

A much better and more robust fix would be to fix AutoClosingTransactionManager.close() to be idempotent. TransactionManager could then implement Closeable instead of AutoCloseable to denote this (if desired).

[archiecobbs]

Patch...

diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SessionPool.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SessionPool.java
index 8c60e6a..3f21e28 100644
--- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SessionPool.java
+++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SessionPool.java
@@ -199,6 +199,7 @@ final class SessionPool {
   private static class AutoClosingTransactionManager implements TransactionManager {
     final TransactionManager delegate;
     final PooledSession session;
+    private boolean closed;

     AutoClosingTransactionManager(TransactionManager delegate, PooledSession session) {
       this.delegate = delegate;
@@ -242,6 +243,10 @@ final class SessionPool {

     @Override
     public void close() {
+      if (closed) {
+        return;
+      }
+      closed = true;
       try {
         delegate.close();
       } finally {

This is not thread-safe (should really be using AtomicBoolean.compareAndSet()) but it's consistent with similar existing code.

[vkedia]

@archiecobbs Thanks for reporting this. Your patch looks good. Do you want to send a PR for that?

[archiecobbs]

Sure, added #3183.

[vkedia]

Thanks. Closing this issue now.