From 2678f85506bff99843fd82715c1021aa4ca703e1 Mon Sep 17 00:00:00 2001 From: Roland Praml Date: Sat, 18 May 2019 00:00:34 +0200 Subject: [PATCH] Fix memory leak in threadLocal on implicit transactions (#1711) * refactor TestExecutionComplete * Add more tests to discover missing threadScope cleanups * FIX: clear threadScope on implicit created Jdbc transactions * FIX: clear threadScope on completed JTA transactions * Revert "FIX: clear threadScope on completed JTA transactions" This reverts commit f844405542f03f19b682334951872505676b89dc. --- .../DefaultTransactionScopeManager.java | 4 ++ .../DefaultTransactionThreadLocal.java | 11 ++++ .../server/transaction/JdbcTransaction.java | 3 + .../transaction/TransactionScopeManager.java | 5 ++ .../transaction/TestExecuteComplete.java | 64 +++++++++++++++---- 5 files changed, 75 insertions(+), 12 deletions(-) diff --git a/src/main/java/io/ebeaninternal/server/transaction/DefaultTransactionScopeManager.java b/src/main/java/io/ebeaninternal/server/transaction/DefaultTransactionScopeManager.java index bf949c231..103d75be6 100644 --- a/src/main/java/io/ebeaninternal/server/transaction/DefaultTransactionScopeManager.java +++ b/src/main/java/io/ebeaninternal/server/transaction/DefaultTransactionScopeManager.java @@ -42,5 +42,9 @@ public class DefaultTransactionScopeManager extends TransactionScopeManager { DefaultTransactionThreadLocal.set(serverName, trans); } + @Override + public void clear(SpiTransaction trans) { + DefaultTransactionThreadLocal.clear(serverName, trans); + } } diff --git a/src/main/java/io/ebeaninternal/server/transaction/DefaultTransactionThreadLocal.java b/src/main/java/io/ebeaninternal/server/transaction/DefaultTransactionThreadLocal.java index 4bcd6b9ea..cecd08671 100644 --- a/src/main/java/io/ebeaninternal/server/transaction/DefaultTransactionThreadLocal.java +++ b/src/main/java/io/ebeaninternal/server/transaction/DefaultTransactionThreadLocal.java @@ -40,6 +40,17 @@ public final class DefaultTransactionThreadLocal { } } + /** + * Clears a transaction from the ThreadLocal to prevent memory leaks. + * Will only clear, if trans == currentTransaction + */ + public static void clear(String serverName, SpiTransaction trans) { + Map map = local.get(); + if (map.get(serverName) == trans) { + map.remove(serverName); + } + } + /** * A mechanism to get the transaction out of the thread local by replacing it * with a 'proxy'. diff --git a/src/main/java/io/ebeaninternal/server/transaction/JdbcTransaction.java b/src/main/java/io/ebeaninternal/server/transaction/JdbcTransaction.java index eeb5bbf5b..bec80e982 100644 --- a/src/main/java/io/ebeaninternal/server/transaction/JdbcTransaction.java +++ b/src/main/java/io/ebeaninternal/server/transaction/JdbcTransaction.java @@ -920,6 +920,9 @@ public class JdbcTransaction implements SpiTransaction, TxnProfileEventCodes { } connection = null; active = false; + if (manager != null) { + manager.scope().clear(this); + } profileEnd(); } diff --git a/src/main/java/io/ebeaninternal/server/transaction/TransactionScopeManager.java b/src/main/java/io/ebeaninternal/server/transaction/TransactionScopeManager.java index 61f2b8050..66ae60f33 100644 --- a/src/main/java/io/ebeaninternal/server/transaction/TransactionScopeManager.java +++ b/src/main/java/io/ebeaninternal/server/transaction/TransactionScopeManager.java @@ -34,6 +34,11 @@ public abstract class TransactionScopeManager implements SpiTransactionScopeMana */ public abstract void set(SpiTransaction trans); + /** + * Clears the given Transaction for this serverName and Thread. + */ + public abstract void clear(SpiTransaction trans); + /** * Replace the current transaction with this one. *

diff --git a/src/test/java/org/tests/transaction/TestExecuteComplete.java b/src/test/java/org/tests/transaction/TestExecuteComplete.java index 4b880cfb9..54c3661e8 100644 --- a/src/test/java/org/tests/transaction/TestExecuteComplete.java +++ b/src/test/java/org/tests/transaction/TestExecuteComplete.java @@ -1,8 +1,8 @@ package org.tests.transaction; import io.ebean.BaseTestCase; +import io.ebean.DB; import io.ebean.DataIntegrityException; -import io.ebean.Ebean; import io.ebean.Transaction; import io.ebean.TxScope; import io.ebean.annotation.ForPlatform; @@ -16,7 +16,7 @@ import org.tests.model.basic.Customer; import org.tests.model.basic.Order; import static org.assertj.core.api.StrictAssertions.assertThat; -import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; public class TestExecuteComplete extends BaseTestCase { @@ -26,15 +26,15 @@ public class TestExecuteComplete extends BaseTestCase { public void execute_when_errorOnCommit_threadLocalIsCleared() { try { - Ebean.execute(TxScope.required().setBatch(PersistBatch.ALL), () -> { + DB.execute(TxScope.required().setBatch(PersistBatch.ALL), () -> { - Customer customer = Ebean.getReference(Customer.class, 42424242L); + Customer customer = DB.getReference(Customer.class, 42424242L); Order order = new Order(); order.setCustomer(customer); - Ebean.save(order); + DB.save(order); }); - assertTrue(false); + fail(); } catch (DataIntegrityException e) { // assert the thread local has been cleaned up SpiTransaction txn = DefaultTransactionThreadLocal.get("h2"); @@ -47,15 +47,16 @@ public class TestExecuteComplete extends BaseTestCase { public void nestedExecute_when_errorOnCommit_threadLocalIsCleared() { try { - Ebean.execute(TxScope.required().setBatch(PersistBatch.ALL), () -> - Ebean.execute(() -> { + DB.execute(TxScope.required().setBatch(PersistBatch.ALL), () -> + DB.execute(() -> { - Customer customer = Ebean.getReference(Customer.class, 42424242L); + Customer customer = DB.getReference(Customer.class, 42424242L); Order order = new Order(); order.setCustomer(customer); - Ebean.save(order); + DB.save(order); })); + fail(); } catch (DataIntegrityException e) { // assert the thread local has been cleaned up SpiTransaction txn = DefaultTransactionThreadLocal.get("h2"); @@ -69,6 +70,7 @@ public class TestExecuteComplete extends BaseTestCase { try { errorOnCommit(); + fail(); } catch (DataIntegrityException e) { SpiTransaction txn = DefaultTransactionThreadLocal.get("h2"); assertThat(txn).isNull(); @@ -77,11 +79,11 @@ public class TestExecuteComplete extends BaseTestCase { @Transactional(batchSize = 10) private void errorOnCommit() { - Customer customer = Ebean.getReference(Customer.class, 42424242L); + Customer customer = DB.getReference(Customer.class, 42424242L); Order order = new Order(); order.setCustomer(customer); - Ebean.save(order); + DB.save(order); } @ForPlatform(Platform.H2) @@ -131,4 +133,42 @@ public class TestExecuteComplete extends BaseTestCase { assertThat(txn2).isNull(); } + @ForPlatform(Platform.H2) + @Test + public void implicit_query_expect_threadScopeCleanup() { + + DB.find(Customer.class).findList(); + + SpiTransaction txn = DefaultTransactionThreadLocal.get("h2"); + assertThat(txn).isNull(); + } + + @ForPlatform(Platform.H2) + @Test + public void implicit_save_expect_threadScopeCleanup() { + + Customer cust = new Customer(); + cust.setName("Roland"); + DB.save(cust); + + SpiTransaction txn = DefaultTransactionThreadLocal.get("h2"); + assertThat(txn).isNull(); + } + + @ForPlatform(Platform.H2) + @Test + public void no_transaction_expect_threadScopeCleanup() { + + try (Transaction txn = DB.beginTransaction(TxScope.notSupported())) { + SpiTransaction txn2 = DefaultTransactionThreadLocal.get("h2"); + // 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(); + } + }