From 55800040505ef6ec33d05d88173207262dc27a1e Mon Sep 17 00:00:00 2001 From: Joe Wicentowski Date: Tue, 24 Mar 2026 23:26:10 -0400 Subject: [PATCH 1/3] [bugfix] Fix ExecutorService thread leak in TransactionTestDSL MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit TransactionTestDSL.execute() creates two single-thread ExecutorService instances but only shuts them down in the catch block (error path). On the happy path, both executors leak — their non-daemon threads accumulate across test classes in the same surefire fork and prevent clean JVM exit. Move shutdownNow() calls to a finally block so executors are always cleaned up regardless of success or failure. Found via thread dump during full test suite run: 10 leaked "transaction-test-dsl" threads from 5 ConcurrentTransactionsTest invocations were still alive at fork shutdown. Co-Authored-By: Claude Opus 4.6 (1M context) --- .../main/java/org/exist/test/TransactionTestDSL.java | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/exist-core/src/main/java/org/exist/test/TransactionTestDSL.java b/exist-core/src/main/java/org/exist/test/TransactionTestDSL.java index 285527e90d2..2c883e28d75 100644 --- a/exist-core/src/main/java/org/exist/test/TransactionTestDSL.java +++ b/exist-core/src/main/java/org/exist/test/TransactionTestDSL.java @@ -535,16 +535,13 @@ public Tuple2 execute(final BrokerPool brokerPool, final ExecutionListen Thread.sleep(50); } } catch (final ExecutionException | InterruptedException e) { - // if we get to here then t1Result or t2Result has thrown an exception - - // force shutdown of transaction threads - - t2ExecutorService.shutdownNow(); - t1ExecutorService.shutdownNow(); - //TODO(AR) rather than working with exceptions, it would be better to encapsulate them in a similar way to working on an empty sequence, e.g. could use Either??? throw e; + } finally { + // always shutdown executor services to prevent thread leaks + t1ExecutorService.shutdownNow(); + t2ExecutorService.shutdownNow(); } } } From 32dd3bca98845c87b62a29fe9952afd2ed90742a Mon Sep 17 00:00:00 2001 From: Joe Wicentowski Date: Wed, 25 Mar 2026 07:33:08 -0400 Subject: [PATCH 2/3] [bugfix] Use try-with-resources for ExecutorService shutdown Apply Patrick's suggestion to use try-with-resources instead of explicit shutdownNow() in a finally block. ExecutorService implements AutoCloseable since Java 19, and close() calls shutdown() followed by awaitTermination(), which is the correct cleanup for test code. Co-Authored-By: Claude Opus 4.6 (1M context) --- .../org/exist/test/TransactionTestDSL.java | 52 ++++++++----------- 1 file changed, 22 insertions(+), 30 deletions(-) diff --git a/exist-core/src/main/java/org/exist/test/TransactionTestDSL.java b/exist-core/src/main/java/org/exist/test/TransactionTestDSL.java index 2c883e28d75..7797107d876 100644 --- a/exist-core/src/main/java/org/exist/test/TransactionTestDSL.java +++ b/exist-core/src/main/java/org/exist/test/TransactionTestDSL.java @@ -493,29 +493,29 @@ public Tuple2 execute(final BrokerPool brokerPool, final ExecutionListen final ThreadGroup transactionsThreadGroup = newInstanceSubThreadGroup(brokerPool, "transactionTestDSL"); - // submit t1 - final ExecutorService t1ExecutorService = Executors.newSingleThreadExecutor(r -> new Thread(transactionsThreadGroup, r, nameInstanceThread(brokerPool, "transaction-test-dsl.transaction-1-schedule"))); - final Future t1Result = t1ExecutorService.submit(() -> { - try (final DBBroker broker = brokerPool.get(Optional.of(brokerPool.getSecurityManager().getSystemSubject())); - final Txn txn = brokerPool.getTransactionManager().beginTransaction()) { - final U1 result = lastOperation.t1_state.apply(broker, txn, executionListener, null); - txn.commit(); - return result; - } - }); - - // submit t2 - final ExecutorService t2ExecutorService = Executors.newSingleThreadExecutor(r -> new Thread(transactionsThreadGroup, r, nameInstanceThread(brokerPool, "transaction-test-dsl.transaction-2-schedule"))); - final Future t2Result = t2ExecutorService.submit(() -> { - try (final DBBroker broker = brokerPool.get(Optional.of(brokerPool.getSecurityManager().getSystemSubject())); - final Txn txn = brokerPool.getTransactionManager().beginTransaction()) { - final U2 result = lastOperation.t2_state.apply(broker, txn, executionListener, null); - txn.commit(); - return result; - } - }); + // submit t1 and t2 — use try-with-resources to ensure executor shutdown (Java 19+) + try (final ExecutorService t1ExecutorService = Executors.newSingleThreadExecutor(r -> new Thread(transactionsThreadGroup, r, nameInstanceThread(brokerPool, "transaction-test-dsl.transaction-1-schedule"))); + final ExecutorService t2ExecutorService = Executors.newSingleThreadExecutor(r -> new Thread(transactionsThreadGroup, r, nameInstanceThread(brokerPool, "transaction-test-dsl.transaction-2-schedule")))) { + + final Future t1Result = t1ExecutorService.submit(() -> { + try (final DBBroker broker = brokerPool.get(Optional.of(brokerPool.getSecurityManager().getSystemSubject())); + final Txn txn = brokerPool.getTransactionManager().beginTransaction()) { + final U1 result = lastOperation.t1_state.apply(broker, txn, executionListener, null); + txn.commit(); + return result; + } + }); + + final Future t2Result = t2ExecutorService.submit(() -> { + try (final DBBroker broker = brokerPool.get(Optional.of(brokerPool.getSecurityManager().getSystemSubject())); + final Txn txn = brokerPool.getTransactionManager().beginTransaction()) { + final U2 result = lastOperation.t2_state.apply(broker, txn, executionListener, null); + txn.commit(); + return result; + } + }); - try { + //TODO(AR) rather than working with exceptions, it would be better to encapsulate them in a similar way to working on an empty sequence, e.g. could use Either??? U1 u1 = null; U2 u2 = null; @@ -534,14 +534,6 @@ public Tuple2 execute(final BrokerPool brokerPool, final ExecutionListen Thread.sleep(50); } - } catch (final ExecutionException | InterruptedException e) { - //TODO(AR) rather than working with exceptions, it would be better to encapsulate them in a similar way to working on an empty sequence, e.g. could use Either??? - - throw e; - } finally { - // always shutdown executor services to prevent thread leaks - t1ExecutorService.shutdownNow(); - t2ExecutorService.shutdownNow(); } } } From 14bb9e0f202e945e6399ff74b7d5b60847ef57f2 Mon Sep 17 00:00:00 2001 From: Joe Wicentowski Date: Wed, 25 Mar 2026 11:33:51 -0400 Subject: [PATCH 3/3] [bugfix] Clarify TODO comment context after try-with-resources refactor The TODO(AR) about using Either instead of exceptions was originally in the catch block. Since the catch block was removed in the try-with-resources refactor, add "from Future.get()" to clarify what exceptions the TODO refers to. Co-Authored-By: Claude Opus 4.6 (1M context) --- .../src/main/java/org/exist/test/TransactionTestDSL.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/exist-core/src/main/java/org/exist/test/TransactionTestDSL.java b/exist-core/src/main/java/org/exist/test/TransactionTestDSL.java index 7797107d876..866dbd42d22 100644 --- a/exist-core/src/main/java/org/exist/test/TransactionTestDSL.java +++ b/exist-core/src/main/java/org/exist/test/TransactionTestDSL.java @@ -515,8 +515,7 @@ public Tuple2 execute(final BrokerPool brokerPool, final ExecutionListen } }); - //TODO(AR) rather than working with exceptions, it would be better to encapsulate them in a similar way to working on an empty sequence, e.g. could use Either??? - + //TODO(AR) rather than working with exceptions from Future.get(), it would be better to encapsulate them in a similar way to working on an empty sequence, e.g. could use Either??? U1 u1 = null; U2 u2 = null; while (true) {