Fix ExecutorService thread leak in TransactionTestDSL#6175
Merged
duncdrum merged 3 commits intoeXist-db:developfrom Mar 25, 2026
Merged
Fix ExecutorService thread leak in TransactionTestDSL#6175duncdrum merged 3 commits intoeXist-db:developfrom
duncdrum merged 3 commits intoeXist-db:developfrom
Conversation
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) <noreply@anthropic.com>
reinhapa
reviewed
Mar 25, 2026
exist-core/src/main/java/org/exist/test/TransactionTestDSL.java
Outdated
Show resolved
Hide resolved
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) <noreply@anthropic.com>
reinhapa
approved these changes
Mar 25, 2026
line-o
reviewed
Mar 25, 2026
| } | ||
| }); | ||
|
|
||
| //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<L,R>??? |
Member
There was a problem hiding this comment.
Is this comment still relevant after the code changes?
The TODO(AR) about using Either<L,R> 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) <noreply@anthropic.com>
duncdrum
approved these changes
Mar 25, 2026
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
TransactionTestDSL.execute()creates twoExecutorServiceinstances but only shuts them down in thecatchblock (error path). On the happy path, both executors leak — their non-daemon threads accumulate across test classes in the same surefire fork.The fix moves
shutdownNow()to afinallyblock.How it was found
Thread dump captured during a full
mvn test -pl exist-corerun showed 10 leakeddb.exist.transaction-test-dsl.transaction-*-schedulethreads from 5ConcurrentTransactionsTestinvocations, all inWAITING (parking)state atThreadPoolExecutor.getTask(). These non-daemon threads prevented clean JVM exit after all tests completed.What changed
TransactionTestDSL.java— Movedt1ExecutorService.shutdownNow()andt2ExecutorService.shutdownNow()fromcatchtofinallyso executor cleanup happens on both success and error paths.Related
MoveResourceTeststill triggers mid-testMultiLockcontention on concurrent collection move + query. This is real lock contention in the engine, not a test infrastructure issue. PR Preclaiming two-phase locking for XQuery update operations #6112 (preclaiming locks) resolves it — full test suite completes in 3:26 with Preclaiming two-phase locking for XQuery update operations #6112 vs hanging without it.Test plan
mvn test -pl exist-core— 6533 tests, 0 failures, BUILD SUCCESS🤖 Generated with Claude Code