#1347 - Improve cleanup of inactive transactions from threadLocal when no end() used

src/main/java/io/ebeaninternal/api/ScopedTransaction.java

@@ -21,6 +21,11 @@ public class ScopedTransaction extends SpiTransactionProxy {
 
   private ScopeTrans current;
 
+  /**
+   * Flag set when we clear the thread scope (on commit/rollback or end).
+   */
+  private boolean scopeCleared;
+
   public ScopedTransaction(TransactionScopeManager manager) {
     this.manager = manager;
   }
@@ -47,30 +52,51 @@ public class ScopedTransaction extends SpiTransactionProxy {
    */
   public void complete(Object returnOrThrowable, int opCode) {
     current.complete(returnOrThrowable, opCode);
+    // no finally here for pop() as we come in here twice if an
+    // error is thrown on commit (due to enhancement finally block)
     pop();
   }
 
   /**
-   * Programmatic complete - finally block, try to commit.
+   * Internal programmatic complete - finally block, try to commit.
    */
   public void complete() {
-    current.complete();
-    pop();
+    try {
+      current.complete();
+    } finally {
+      pop();
+    }
+  }
+
+  private void clearScopeOnce() {
+    if (!scopeCleared) {
+      manager.set(null);
+      scopeCleared = true;
+    }
+  }
+
+  private boolean clearScope() {
+    if (stack.isEmpty()) {
+      clearScopeOnce();
+      return true;
+    }
+    return false;
   }
 
   private void pop() {
-    if (!stack.isEmpty()) {
+    if (!clearScope()) {
       current = stack.pop();
       transaction = current.getTransaction();
-    } else {
-      manager.set(null);
     }
   }
 
   @Override
   public void end() throws PersistenceException {
-    current.end();
-    pop();
+    try {
+      current.end();
+    } finally {
+      pop();
+    }
   }
 
   @Override
@@ -80,17 +106,29 @@ public class ScopedTransaction extends SpiTransactionProxy {
 
   @Override
   public void commit() {
-    current.commitTransaction();
+    try {
+      current.commitTransaction();
+    } finally {
+      clearScope();
+    }
   }
 
   @Override
   public void rollback() throws PersistenceException {
-    current.rollback(null);
+    try {
+      current.rollback(null);
+    } finally {
+      clearScope();
+    }
   }
 
   @Override
   public void rollback(Throwable e) throws PersistenceException {
-    current.rollback(e);
+    try {
+      current.rollback(e);
+    } finally {
+      clearScope();
+    }
   }
 
   @Override

src/test/java/org/tests/inheritance/TestDuplicateKeyException.java

@@ -3,11 +3,11 @@ package org.tests.inheritance;
 
 import io.ebean.BaseTestCase;
 import io.ebean.Ebean;
+import org.junit.Assert;
+import org.junit.Test;
 import org.tests.model.basic.AttributeHolder;
 import org.tests.model.basic.ListAttribute;
 import org.tests.model.basic.ListAttributeValue;
-import org.junit.Assert;
-import org.junit.Test;
 
 public class TestDuplicateKeyException extends BaseTestCase {
 

src/test/java/org/tests/transaction/TestExecuteComplete.java

@@ -0,0 +1,132 @@
+package org.tests.transaction;
+
+import io.ebean.BaseTestCase;
+import io.ebean.DataIntegrityException;
+import io.ebean.Ebean;
+import io.ebean.Transaction;
+import io.ebean.TxScope;
+import io.ebean.annotation.ForPlatform;
+import io.ebean.annotation.PersistBatch;
+import io.ebean.annotation.Platform;
+import io.ebean.annotation.Transactional;
+import io.ebeaninternal.api.SpiTransaction;
+import io.ebeaninternal.server.transaction.DefaultTransactionThreadLocal;
+import org.junit.Test;
+import org.tests.model.basic.Customer;
+import org.tests.model.basic.Order;
+
+import static org.assertj.core.api.StrictAssertions.assertThat;
+
+public class TestExecuteComplete extends BaseTestCase {
+
+
+  @ForPlatform(Platform.H2)
+  @Test
+  public void execute_when_errorOnCommit_threadLocalIsCleared() {
+
+    try {
+      Ebean.execute(TxScope.required().setBatch(PersistBatch.ALL), () -> {
+
+        Customer customer = Ebean.getReference(Customer.class, 42424242L);
+        Order order = new Order();
+        order.setCustomer(customer);
+
+        Ebean.save(customer);
+      });
+    } catch (DataIntegrityException e) {
+      // assert the thread local has been cleaned up
+      SpiTransaction txn = DefaultTransactionThreadLocal.get("h2");
+      assertThat(txn).isNull();
+    }
+  }
+
+  @ForPlatform(Platform.H2)
+  @Test
+  public void nestedExecute_when_errorOnCommit_threadLocalIsCleared() {
+
+    try {
+      Ebean.execute(TxScope.required().setBatch(PersistBatch.ALL), () ->
+        Ebean.execute(() -> {
+
+          Customer customer = Ebean.getReference(Customer.class, 42424242L);
+          Order order = new Order();
+          order.setCustomer(customer);
+
+          Ebean.save(customer);
+        }));
+    } catch (DataIntegrityException e) {
+      // assert the thread local has been cleaned up
+      SpiTransaction txn = DefaultTransactionThreadLocal.get("h2");
+      assertThat(txn).isNull();
+    }
+  }
+
+  @ForPlatform(Platform.H2)
+  @Test
+  public void transactional_errorOnCommit_expect_threadScopeCleanup() {
+
+    try {
+      errorOnCommit();
+    } catch (DataIntegrityException e) {
+      SpiTransaction txn = DefaultTransactionThreadLocal.get("h2");
+      assertThat(txn).isNull();
+    }
+  }
+
+  @Transactional(batchSize = 10)
+  private void errorOnCommit() {
+    Customer customer = Ebean.getReference(Customer.class, 42424242L);
+    Order order = new Order();
+    order.setCustomer(customer);
+
+    Ebean.save(customer);
+  }
+
+  @ForPlatform(Platform.H2)
+  @Test
+  public void normal_expect_threadScopeCleanup() {
+
+    Transaction txn1 = server().beginTransaction();
+    try {
+      txn1.commit();
+    } finally {
+      txn1.end();
+    }
+
+    SpiTransaction txn2 = DefaultTransactionThreadLocal.get("h2");
+    assertThat(txn2).isNull();
+  }
+
+  @ForPlatform(Platform.H2)
+  @Test
+  public void missingEnd_expect_threadScopeCleanup() {
+
+    Transaction txn1 = server().beginTransaction();
+    try {
+      txn1.commit();
+    } finally {
+      // accidentally omit end()
+      //txn1.end();
+    }
+
+    SpiTransaction txn2 = DefaultTransactionThreadLocal.get("h2");
+    assertThat(txn2).isNull();
+  }
+
+  @ForPlatform(Platform.H2)
+  @Test
+  public void missingEnd_withRollbackOnly_expect_threadScopeCleanup() {
+
+    Transaction txn1 = server().beginTransaction();
+    try {
+      txn1.rollback();
+    } finally {
+      // accidentally omit end()
+      //txn1.end();
+    }
+
+    SpiTransaction txn2 = DefaultTransactionThreadLocal.get("h2");
+    assertThat(txn2).isNull();
+  }
+
+}