From 607cf3156c472a294799d4ea8496277d55bc23d8 Mon Sep 17 00:00:00 2001 From: Rob Bygrave Date: Tue, 13 Mar 2018 00:15:42 +1300 Subject: [PATCH] #1347 - Improve cleanup of inactive transactions from threadLocal when no end() used --- .../ebeaninternal/api/ScopedTransaction.java | 60 ++++++-- .../TestDuplicateKeyException.java | 4 +- .../transaction/TestExecuteComplete.java | 132 ++++++++++++++++++ 3 files changed, 183 insertions(+), 13 deletions(-) create mode 100644 src/test/java/org/tests/transaction/TestExecuteComplete.java diff --git a/src/main/java/io/ebeaninternal/api/ScopedTransaction.java b/src/main/java/io/ebeaninternal/api/ScopedTransaction.java index 833a3d7b6..a8d38b03f 100644 --- a/src/main/java/io/ebeaninternal/api/ScopedTransaction.java +++ b/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 diff --git a/src/test/java/org/tests/inheritance/TestDuplicateKeyException.java b/src/test/java/org/tests/inheritance/TestDuplicateKeyException.java index 987cb5043..2b53a3d4a 100644 --- a/src/test/java/org/tests/inheritance/TestDuplicateKeyException.java +++ b/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 { diff --git a/src/test/java/org/tests/transaction/TestExecuteComplete.java b/src/test/java/org/tests/transaction/TestExecuteComplete.java new file mode 100644 index 000000000..a8ec5f8c3 --- /dev/null +++ b/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(); + } + +}