From c6ba4d8155cd8e5dd93181777722485de87ec8d7 Mon Sep 17 00:00:00 2001 From: Rob Bygrave Date: Sat, 18 May 2019 15:16:17 +1200 Subject: [PATCH] Refactor DefaultTransactionThreadLocal, convert ThreadLocal to field (from map) (#1716) * Refactor DefaultTransactionThreadLocal, convert ThreadLocal to field (from map) * Add test for multiple database thread locals --- .../api/SpiTransactionManager.java | 6 ++ .../DefaultTransactionScopeManager.java | 33 ++++++-- .../DefaultTransactionThreadLocal.java | 84 ------------------- .../transaction/JtaTransactionManager.java | 18 ++-- .../transaction/TransactionManager.java | 2 +- .../transaction/TransactionScopeManager.java | 6 ++ src/test/java/io/ebean/BaseTestCase.java | 19 +++-- .../DefaultTransactionThreadLocalTest.java | 63 +++++++++++--- .../transaction/TestExecuteComplete.java | 30 +++---- 9 files changed, 120 insertions(+), 141 deletions(-) delete mode 100644 src/main/java/io/ebeaninternal/server/transaction/DefaultTransactionThreadLocal.java diff --git a/src/main/java/io/ebeaninternal/api/SpiTransactionManager.java b/src/main/java/io/ebeaninternal/api/SpiTransactionManager.java index d08e5aa2b..d2c23ee67 100644 --- a/src/main/java/io/ebeaninternal/api/SpiTransactionManager.java +++ b/src/main/java/io/ebeaninternal/api/SpiTransactionManager.java @@ -1,6 +1,7 @@ package io.ebeaninternal.api; import io.ebean.TxScope; +import io.ebeaninternal.server.transaction.TransactionScopeManager; import javax.sql.DataSource; @@ -9,6 +10,11 @@ import javax.sql.DataSource; */ public interface SpiTransactionManager { + /** + * Return the scope manager for this server. + */ + TransactionScopeManager scope(); + /** * Return the main DataSource. */ diff --git a/src/main/java/io/ebeaninternal/server/transaction/DefaultTransactionScopeManager.java b/src/main/java/io/ebeaninternal/server/transaction/DefaultTransactionScopeManager.java index 4a4ca8214..0134ff5ed 100644 --- a/src/main/java/io/ebeaninternal/server/transaction/DefaultTransactionScopeManager.java +++ b/src/main/java/io/ebeaninternal/server/transaction/DefaultTransactionScopeManager.java @@ -2,12 +2,16 @@ package io.ebeaninternal.server.transaction; import io.ebeaninternal.api.SpiTransaction; +import javax.persistence.PersistenceException; + /** * Manages the transaction scoping using a Ebean thread local. */ public class DefaultTransactionScopeManager extends TransactionScopeManager { + private final ThreadLocal local = new ThreadLocal<>(); + public DefaultTransactionScopeManager(String serverName) { super(serverName); } @@ -19,12 +23,12 @@ public class DefaultTransactionScopeManager extends TransactionScopeManager { @Override public SpiTransaction getInScope() { - return DefaultTransactionThreadLocal.get(serverName); + return local.get(); } @Override public SpiTransaction getActive() { - SpiTransaction t = DefaultTransactionThreadLocal.get(serverName); + SpiTransaction t = local.get(); if (t == null || !t.isActive()) { return null; } else { @@ -34,17 +38,36 @@ public class DefaultTransactionScopeManager extends TransactionScopeManager { @Override public void replace(SpiTransaction trans) { - DefaultTransactionThreadLocal.replace(serverName, trans); + if (trans == null) { + throw new IllegalStateException("Setting a null transaction?"); + } + local.set(trans); } @Override public void set(SpiTransaction trans) { - DefaultTransactionThreadLocal.set(serverName, trans); + if (trans == null) { + throw new IllegalStateException("Setting a null transaction?"); + } + checkForActiveTransaction(); + local.set(trans); } @Override public void clear() { - DefaultTransactionThreadLocal.clear(serverName); + checkForActiveTransaction(); + local.remove(); } + @Override + public void clearExternal() { + local.remove(); + } + + private void checkForActiveTransaction() { + SpiTransaction transaction = local.get(); + if (transaction != null && transaction.isActive()) { + throw new PersistenceException("Invalid state - there is an existing Active transaction " + transaction); + } + } } diff --git a/src/main/java/io/ebeaninternal/server/transaction/DefaultTransactionThreadLocal.java b/src/main/java/io/ebeaninternal/server/transaction/DefaultTransactionThreadLocal.java deleted file mode 100644 index 9b415ece5..000000000 --- a/src/main/java/io/ebeaninternal/server/transaction/DefaultTransactionThreadLocal.java +++ /dev/null @@ -1,84 +0,0 @@ -package io.ebeaninternal.server.transaction; - -import io.ebeaninternal.api.SpiTransaction; - -import javax.persistence.PersistenceException; -import java.util.HashMap; -import java.util.Map; - -/** - * Used to store Transactions in a ThreadLocal. - */ -public final class DefaultTransactionThreadLocal { - - private static final ThreadLocal> local = ThreadLocal.withInitial(HashMap::new); - - /** - * Not allowed. - */ - private DefaultTransactionThreadLocal() { - } - - /** - * Remove the transaction entry for the given serverName. - */ - private static void remove(String serverName) { - local.get().remove(serverName); - } - - /** - * Set a new Transaction for this serverName and Thread. - */ - public static void set(String serverName, SpiTransaction trans) { - if (trans == null) { - throw new IllegalStateException("Setting a null transaction?"); - } - SpiTransaction existingTransaction = local.get().put(serverName, trans); - if (existingTransaction != null && existingTransaction.isActive()) { - throw new PersistenceException("The existing transaction is still active?"); - } - } - - /** - * Clear a transaction. It should be inactive. - */ - public static void clear(String serverName) { - SpiTransaction transaction = local.get().remove(serverName); - if (transaction != null && transaction.isActive()) { - throw new IllegalStateException("Clearing an ACTIVE transaction " + transaction); - } - } - - /** - * A mechanism to get the transaction out of the thread local by replacing it - * with a 'proxy'. - *

- * Used for background fetching. Replaces the current transaction with a - * 'dummy' transaction. The current transaction is given to the background - * thread so it can continue the fetch. - *

- */ - public static void replace(String serverName, SpiTransaction trans) { - if (trans == null) { - remove(serverName); - } else { - local.get().put(serverName, trans); - } - } - - /** - * Return the current Transaction for this serverName and Thread. - */ - public static SpiTransaction get(String serverName) { - return local.get().get(serverName); - } - - /** - * Return all transactions of the current thread (active/inactive). - * This is intended for test/debugging purposes only! - */ - public static Map currentTransactions() { - return local.get(); - } - -} diff --git a/src/main/java/io/ebeaninternal/server/transaction/JtaTransactionManager.java b/src/main/java/io/ebeaninternal/server/transaction/JtaTransactionManager.java index 1933b10c6..0bfd76e4f 100644 --- a/src/main/java/io/ebeaninternal/server/transaction/JtaTransactionManager.java +++ b/src/main/java/io/ebeaninternal/server/transaction/JtaTransactionManager.java @@ -34,10 +34,7 @@ public class JtaTransactionManager implements ExternalTransactionManager { */ private TransactionManager transactionManager; - /** - * The EbeanServer name. - */ - private String serverName; + private TransactionScopeManager scope; /** * Instantiates a new spring aware transaction scope manager. @@ -55,7 +52,7 @@ public class JtaTransactionManager implements ExternalTransactionManager { // the public API and hence the Object type and casting here this.transactionManager = (TransactionManager) txnMgr; - this.serverName = transactionManager.getServerName(); + this.scope = transactionManager.scope(); } /** @@ -102,7 +99,7 @@ public class JtaTransactionManager implements ExternalTransactionManager { } // check current Ebean transaction - SpiTransaction currentEbeanTransaction = DefaultTransactionThreadLocal.get(serverName); + SpiTransaction currentEbeanTransaction = scope.getInScope(); if (currentEbeanTransaction != null) { // NOT expecting this so log WARNING String msg = "JTA Transaction - no current txn BUT using current Ebean one " + currentEbeanTransaction.getId(); @@ -132,7 +129,7 @@ public class JtaTransactionManager implements ExternalTransactionManager { syncRegistry.registerInterposedSynchronization(txnListener); // also put in Ebean ThreadLocal - DefaultTransactionThreadLocal.set(serverName, newTrans); + scope.set(newTrans); return newTrans; } @@ -192,12 +189,9 @@ public class JtaTransactionManager implements ExternalTransactionManager { private final SpiTransaction transaction; - private final String serverName; - private JtaTxnListener(TransactionManager transactionManager, SpiTransaction t) { this.transactionManager = transactionManager; this.transaction = t; - this.serverName = transactionManager.getServerName(); } @Override @@ -216,7 +210,7 @@ public class JtaTransactionManager implements ExternalTransactionManager { } transactionManager.notifyOfCommit(transaction); // Remove this transaction object as it is completed - DefaultTransactionThreadLocal.replace(serverName, null); + transactionManager.scope().clearExternal(); break; case Status.STATUS_ROLLEDBACK: @@ -225,7 +219,7 @@ public class JtaTransactionManager implements ExternalTransactionManager { } transactionManager.notifyOfRollback(transaction, null); // Remove this transaction object as it is completed - DefaultTransactionThreadLocal.replace(serverName, null); + transactionManager.scope().clearExternal(); break; default: diff --git a/src/main/java/io/ebeaninternal/server/transaction/TransactionManager.java b/src/main/java/io/ebeaninternal/server/transaction/TransactionManager.java index e446a9477..14d4e7d72 100644 --- a/src/main/java/io/ebeaninternal/server/transaction/TransactionManager.java +++ b/src/main/java/io/ebeaninternal/server/transaction/TransactionManager.java @@ -597,7 +597,7 @@ public class TransactionManager implements SpiTransactionManager { @Override public void externalRemoveTransaction() { - scopeManager.replace(null); + scopeManager.clearExternal(); } /** diff --git a/src/main/java/io/ebeaninternal/server/transaction/TransactionScopeManager.java b/src/main/java/io/ebeaninternal/server/transaction/TransactionScopeManager.java index 98c192a6c..ac5ce6616 100644 --- a/src/main/java/io/ebeaninternal/server/transaction/TransactionScopeManager.java +++ b/src/main/java/io/ebeaninternal/server/transaction/TransactionScopeManager.java @@ -39,6 +39,12 @@ public abstract class TransactionScopeManager implements SpiTransactionScopeMana */ public abstract void clear(); + /** + * Clears the current Transaction from thread local scope without any check for active + * transactions. Intended for use with external transactions. + */ + public abstract void clearExternal(); + /** * Replace the current transaction with this one. *

diff --git a/src/test/java/io/ebean/BaseTestCase.java b/src/test/java/io/ebean/BaseTestCase.java index ce5da0768..c03988613 100644 --- a/src/test/java/io/ebean/BaseTestCase.java +++ b/src/test/java/io/ebean/BaseTestCase.java @@ -15,8 +15,7 @@ import io.ebeaninternal.server.core.OrmQueryRequest; import io.ebeaninternal.server.deploy.BeanDescriptor; import io.ebeaninternal.server.expression.platform.DbExpressionHandler; import io.ebeaninternal.server.expression.platform.DbExpressionHandlerFactory; -import io.ebeaninternal.server.transaction.DefaultTransactionThreadLocal; - +import io.ebeaninternal.server.transaction.TransactionScopeManager; import org.avaje.agentloader.AgentLoader; import org.junit.After; import org.junit.Rule; @@ -28,7 +27,6 @@ import org.tests.model.basic.Country; import java.sql.Types; import java.util.List; -import java.util.Map; import java.util.stream.Collectors; import static org.assertj.core.api.Assertions.assertThat; @@ -43,13 +41,12 @@ public abstract class BaseTestCase { @After public void checkForLeak() { - Map trans = DefaultTransactionThreadLocal.currentTransactions(); + TransactionScopeManager scope = spiEbeanServer().getTransactionManager().scope(); + SpiTransaction trans = scope.getInScope(); if (trans != null) { - if (!trans.isEmpty()) { - String msg = getClass().getSimpleName() + "." + name.getMethodName() + " did not clear threadScope:" + trans; - trans.clear(); // clear for next test - fail(msg); - } + String msg = getClass().getSimpleName() + "." + name.getMethodName() + " did not clear threadScope:" + trans; + scope.clearExternal(); // clear for next test + fail(msg); } } @@ -104,6 +101,10 @@ public abstract class BaseTestCase { .collect(Collectors.toList()); } + protected SpiTransaction getInScopeTransaction() { + return spiEbeanServer().getTransactionManager().scope().getInScope(); + } + /** * Return the generated sql trimming column alias if required. */ diff --git a/src/test/java/io/ebeaninternal/server/transaction/DefaultTransactionThreadLocalTest.java b/src/test/java/io/ebeaninternal/server/transaction/DefaultTransactionThreadLocalTest.java index b235b56a6..a3f95c8ed 100644 --- a/src/test/java/io/ebeaninternal/server/transaction/DefaultTransactionThreadLocalTest.java +++ b/src/test/java/io/ebeaninternal/server/transaction/DefaultTransactionThreadLocalTest.java @@ -1,13 +1,21 @@ package io.ebeaninternal.server.transaction; import io.ebean.BaseTestCase; +import io.ebean.DB; +import io.ebean.Database; +import io.ebean.DatabaseFactory; import io.ebean.Ebean; import io.ebean.Transaction; import io.ebean.annotation.ForPlatform; import io.ebean.annotation.Platform; +import io.ebean.config.DatabaseConfig; import io.ebeaninternal.api.SpiTransaction; import org.junit.Test; +import org.tests.model.basic.EBasicVer; +import org.tests.model.basic.UTDetail; +import org.tests.model.basic.UTMaster; +import static org.assertj.core.api.StrictAssertions.assertThat; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertSame; @@ -18,12 +26,12 @@ public class DefaultTransactionThreadLocalTest extends BaseTestCase { @Test public void get() { Ebean.execute(() -> { - SpiTransaction txn = DefaultTransactionThreadLocal.get("h2"); + SpiTransaction txn = getInScopeTransaction(); assertNotNull(txn); }); // thread local should be set to null - assertNull(DefaultTransactionThreadLocal.get("h2")); + assertNull(getInScopeTransaction()); } @ForPlatform({Platform.H2}) @@ -32,19 +40,19 @@ public class DefaultTransactionThreadLocalTest extends BaseTestCase { try (Transaction transaction = Ebean.beginTransaction()) { assertNotNull(transaction); - SpiTransaction txn = DefaultTransactionThreadLocal.get("h2"); + SpiTransaction txn = getInScopeTransaction(); assertSame(txn, transaction); try (Transaction nested = Ebean.beginTransaction()) { assertNotNull(nested); - SpiTransaction txnNested = DefaultTransactionThreadLocal.get("h2"); + SpiTransaction txnNested = getInScopeTransaction(); assertSame(txnNested, nested); } - assertNotNull(DefaultTransactionThreadLocal.get("h2")); + assertNotNull(getInScopeTransaction()); } - assertNull(DefaultTransactionThreadLocal.get("h2")); + assertNull(getInScopeTransaction()); } @ForPlatform({Platform.H2}) @@ -53,9 +61,9 @@ public class DefaultTransactionThreadLocalTest extends BaseTestCase { try (Transaction transaction = Ebean.beginTransaction()) { transaction.commit(); - assertNull(DefaultTransactionThreadLocal.get("h2")); + assertNull(getInScopeTransaction()); } - assertNull(DefaultTransactionThreadLocal.get("h2")); + assertNull(getInScopeTransaction()); } @ForPlatform({Platform.H2}) @@ -65,9 +73,9 @@ public class DefaultTransactionThreadLocalTest extends BaseTestCase { try (Transaction transaction = Ebean.beginTransaction()) { transaction.rollback(); - assertNull(DefaultTransactionThreadLocal.get("h2")); + assertNull(getInScopeTransaction()); } - assertNull(DefaultTransactionThreadLocal.get("h2")); + assertNull(getInScopeTransaction()); } @ForPlatform({Platform.H2}) @@ -78,4 +86,39 @@ public class DefaultTransactionThreadLocalTest extends BaseTestCase { Ebean.endTransaction(); } + @ForPlatform({Platform.H2}) + @Test + public void multi_database_threadlocals() { + + Database db = DB.getDefault(); + Database otherDb = createOtherDatabase(); + try { + try (Transaction dbTxn = db.beginTransaction()) { + try (Transaction otherDbTxn = otherDb.beginTransaction()) { + assertThat(dbTxn).isNotSameAs(otherDbTxn); + } + } + } finally { + otherDb.shutdown(true, false); + } + } + + + private Database createOtherDatabase() { + + DatabaseConfig config = new DatabaseConfig(); + config.setName("h2ebasicver"); + config.loadFromProperties(); + config.setDdlGenerate(true); + config.setDdlRun(true); + config.setDdlExtra(false); + + config.setRegister(false); + config.setDefaultServer(false); + config.getClasses().add(EBasicVer.class); + config.getClasses().add(UTMaster.class); + config.getClasses().add(UTDetail.class); + + return DatabaseFactory.create(config); + } } diff --git a/src/test/java/org/tests/transaction/TestExecuteComplete.java b/src/test/java/org/tests/transaction/TestExecuteComplete.java index 54c3661e8..4847265f4 100644 --- a/src/test/java/org/tests/transaction/TestExecuteComplete.java +++ b/src/test/java/org/tests/transaction/TestExecuteComplete.java @@ -10,7 +10,6 @@ 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; @@ -37,8 +36,7 @@ public class TestExecuteComplete extends BaseTestCase { fail(); } catch (DataIntegrityException e) { // assert the thread local has been cleaned up - SpiTransaction txn = DefaultTransactionThreadLocal.get("h2"); - assertThat(txn).isNull(); + assertThat(getInScopeTransaction()).isNull(); } } @@ -59,8 +57,7 @@ public class TestExecuteComplete extends BaseTestCase { fail(); } catch (DataIntegrityException e) { // assert the thread local has been cleaned up - SpiTransaction txn = DefaultTransactionThreadLocal.get("h2"); - assertThat(txn).isNull(); + assertThat(getInScopeTransaction()).isNull(); } } @@ -72,8 +69,7 @@ public class TestExecuteComplete extends BaseTestCase { errorOnCommit(); fail(); } catch (DataIntegrityException e) { - SpiTransaction txn = DefaultTransactionThreadLocal.get("h2"); - assertThat(txn).isNull(); + assertThat(getInScopeTransaction()).isNull(); } } @@ -97,8 +93,7 @@ public class TestExecuteComplete extends BaseTestCase { txn1.end(); } - SpiTransaction txn2 = DefaultTransactionThreadLocal.get("h2"); - assertThat(txn2).isNull(); + assertThat(getInScopeTransaction()).isNull(); } @ForPlatform(Platform.H2) @@ -113,8 +108,7 @@ public class TestExecuteComplete extends BaseTestCase { //txn1.end(); } - SpiTransaction txn2 = DefaultTransactionThreadLocal.get("h2"); - assertThat(txn2).isNull(); + assertThat(getInScopeTransaction()).isNull(); } @ForPlatform(Platform.H2) @@ -129,8 +123,7 @@ public class TestExecuteComplete extends BaseTestCase { //txn1.end(); } - SpiTransaction txn2 = DefaultTransactionThreadLocal.get("h2"); - assertThat(txn2).isNull(); + assertThat(getInScopeTransaction()).isNull(); } @ForPlatform(Platform.H2) @@ -139,8 +132,7 @@ public class TestExecuteComplete extends BaseTestCase { DB.find(Customer.class).findList(); - SpiTransaction txn = DefaultTransactionThreadLocal.get("h2"); - assertThat(txn).isNull(); + assertThat(getInScopeTransaction()).isNull(); } @ForPlatform(Platform.H2) @@ -151,8 +143,7 @@ public class TestExecuteComplete extends BaseTestCase { cust.setName("Roland"); DB.save(cust); - SpiTransaction txn = DefaultTransactionThreadLocal.get("h2"); - assertThat(txn).isNull(); + assertThat(getInScopeTransaction()).isNull(); } @ForPlatform(Platform.H2) @@ -160,15 +151,14 @@ public class TestExecuteComplete extends BaseTestCase { public void no_transaction_expect_threadScopeCleanup() { try (Transaction txn = DB.beginTransaction(TxScope.notSupported())) { - SpiTransaction txn2 = DefaultTransactionThreadLocal.get("h2"); + SpiTransaction txn2 = getInScopeTransaction(); // The NoTransaction placeholder can normally only occur inside // a scopedTrans. (Class is package private, so check assertThat(txn2.toString()).contains("NoTransaction"); assertThat(txn2.toString()).contains("NoTransaction"); } - SpiTransaction txn = DefaultTransactionThreadLocal.get("h2"); - assertThat(txn).isNull(); + assertThat(getInScopeTransaction()).isNull(); } }