Make arquillian-core thread-safe for parallel JUnit 5/6 class execution#835
Open
olamy wants to merge 2 commits intoarquillian:mainfrom
Open
Make arquillian-core thread-safe for parallel JUnit 5/6 class execution#835olamy wants to merge 2 commits intoarquillian:mainfrom
olamy wants to merge 2 commits intoarquillian:mainfrom
Conversation
Reviewer's GuideMakes Arquillian core and its JUnit 5 integration safe for parallel class execution by introducing an opt-in per-test-class Manager, switching to real ThreadLocal storage, and tightening thread-safety of container state and manager collections. Sequence diagram for JUnitJupiterTestClassLifecycleManager manager retrievalsequenceDiagram
participant JUnitEngine as JUnitEngine
participant Extension as ArquillianExtension
participant LifecycleMgr as JUnitJupiterTestClassLifecycleManager
participant Context as ExtensionContext
participant Store as ExtensionContextStore
JUnitEngine->>Extension: beforeAll(context)
Extension->>LifecycleMgr: getManager(context)
alt PER_CLASS_MANAGER true
LifecycleMgr->>Context: getRequiredTestClass()
LifecycleMgr->>Context: getRoot()
LifecycleMgr->>Context: getStore(Namespace(JUnitJupiterTestClassLifecycleManager, testClass))
else PER_CLASS_MANAGER false
LifecycleMgr->>Context: getRoot()
LifecycleMgr->>Context: getStore(Namespace(JUnitJupiterTestClassLifecycleManager))
end
LifecycleMgr->>Store: getOrComputeIfAbsent(MANAGER_KEY, factory)
alt first access for MANAGER_KEY
Store-->>LifecycleMgr: create new manager via factory
LifecycleMgr->>LifecycleMgr: initializeAdaptor()
alt initializeAdaptor throws
LifecycleMgr->>LifecycleMgr: caughtInitializationException = e
end
else existing manager
Store-->>LifecycleMgr: existing manager instance
end
LifecycleMgr-->>Extension: manager instance
Extension->>LifecycleMgr: hasInitializationException()
alt initialization failed
Extension->>LifecycleMgr: handleSuiteLevelFailure()
else initialization ok
Extension->>LifecycleMgr: use adaptor for tests
end
JUnitEngine->>Context: root context closed
Context->>LifecycleMgr: close()
LifecycleMgr->>LifecycleMgr: afterSuite() and cleanup
Class diagram for updated concurrency-related classesclassDiagram
class JUnitJupiterTestClassLifecycleManager {
<<extensionResource>>
+static String PER_CLASS_MANAGER_PROPERTY
+static boolean PER_CLASS_MANAGER
-static String MANAGER_KEY
-TestRunnerAdaptor adaptor
-Exception caughtInitializationException
-JUnitJupiterTestClassLifecycleManager()
+static JUnitJupiterTestClassLifecycleManager getManager(ExtensionContext context)
-void initializeAdaptor() throws Exception
-boolean hasInitializationException()
-void handleSuiteLevelFailure()
+void close()
}
class ArquillianThreadLocal~T~ {
-volatile ThreadLocal~T~ delegate
+ArquillianThreadLocal()
-ThreadLocal~T~ newDelegate()
+T initialValue()
+T get()
+void remove()
+void clear()
}
class ContainerImpl~T extends ContainerConfiguration~ {
-DeployableContainer~T~ deployableContainer
-String name
-volatile State state
-volatile Throwable failureCause
-ContainerDef containerConfiguration
}
class ManagerImpl {
-CopyOnWriteArrayList~Context~ contexts
-CopyOnWriteArrayList~Extension~ extensions
-RuntimeLogger runtimeLogger
+ManagerImpl(Collection~Class~ extends Context~~ contextClasses, Collection~Class~~ extensionClasses)
}
class ExtensionContext {
}
class TestRunnerAdaptor {
}
class ContainerConfiguration {
}
class DeployableContainer~T extends ContainerConfiguration~ {
}
class State {
}
class ContainerDef {
}
class Context {
}
class Extension {
}
class RuntimeLogger {
}
JUnitJupiterTestClassLifecycleManager --> TestRunnerAdaptor : owns
JUnitJupiterTestClassLifecycleManager --> ExtensionContext : used in getManager
ArquillianThreadLocal ..|> ThreadLocal : wraps
ContainerImpl --> DeployableContainer : uses
ContainerImpl --> ContainerConfiguration : parameterized by
ContainerImpl --> State : holds state
ContainerImpl --> ContainerDef : configured by
ManagerImpl --> Context : manages
ManagerImpl --> Extension : manages
ManagerImpl --> RuntimeLogger : uses
Flow diagram for ArquillianThreadLocal lifecycle operationsflowchart TD
A[Thread calls get] --> B[delegate.get]
B --> C{Value present for thread?}
C -->|Yes| D[Return existing value]
C -->|No| E[delegate.initialValue]
E --> F[ArquillianThreadLocal.initialValue]
F --> G[Store value in delegate for current thread]
G --> D
H[Thread calls remove] --> I[delegate.remove for current thread]
J[Some component calls clear] --> K[delegate = newDelegate]
K --> L[All threads see fresh ThreadLocal]
L --> M[Next get uses initialValue again]
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
This was referenced Apr 30, 2026
Signed-off-by: Olivier Lamy <olamy@apache.org>
Signed-off-by: Olivier Lamy <olamy@apache.org>
olamy
commented
Apr 30, 2026
| * | ||
| * @see JUnitJupiterTestClassLifecycleManager | ||
| */ | ||
| public static final String PER_CLASS_MANAGER_PROPERTY = "arquillian.junit5.manager.perClass"; |
Contributor
Author
There was a problem hiding this comment.
that's the only big change here and it's not activated per default
Contributor
Author
|
@arquillian/core can you please review this one? The changes are not very significant (maybe except one which is not activated per default). |
Contributor
Author
|
@sourcery-ai maybe update your review with last changes |
|
Sure! I'm generating a new review now. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Using CopyOnWriteArrayList for contexts and extensions avoids ConcurrentModificationException but can be very costly if you add/remove observers frequently; if registration is not strictly one-time at startup, consider a different concurrent collection (e.g. synchronized List or a lock around a plain ArrayList) to avoid repeated array copies under load.
- ArquillianThreadLocal.clear() now swaps in a new ThreadLocal, which leaves the old ThreadLocal instance and its per-thread values to be cleaned up only when threads die; if clear() is called repeatedly in a long-lived process with thread pools, this can accumulate stale ThreadLocals, so you may want to document that clear() is intended to be used sparingly or consider explicitly removing from known threads when feasible.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Using CopyOnWriteArrayList for contexts and extensions avoids ConcurrentModificationException but can be very costly if you add/remove observers frequently; if registration is not strictly one-time at startup, consider a different concurrent collection (e.g. synchronized List or a lock around a plain ArrayList) to avoid repeated array copies under load.
- ArquillianThreadLocal.clear() now swaps in a new ThreadLocal, which leaves the old ThreadLocal instance and its per-thread values to be cleaned up only when threads die; if clear() is called repeatedly in a long-lived process with thread pools, this can accumulate stale ThreadLocals, so you may want to document that clear() is intended to be used sparingly or consider explicitly removing from known threads when feasible.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Hey @olamy, I've posted a new review for you! |
Member
Cool stuff @olamy I believe for #673 the agreement was that the approach is not right - though I don't see now where the conversations happened. I assume this takes a different approach with manager instance where appropriate. It looks like you also tested this in your testsuite successfully, which is great to hear. I ll be off next week, but I ll have a look when I come back |
Contributor
Author
correct. the current changes are less intrusive. (and works, well at least for me :) )
|
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
2 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.
Signed-off-by: Olivier Lamy olamy@apache.org
Short description of what this resolves:
Running JUnit 5/6 with parallel classes against an Arquillian-managed container fails — one Manager is shared across the whole JVM and its context-activation stack is thread-local, so parallel classes
stomp on each other.
Changes proposed in this pull request:
Manager: new system propertyarquillian.junit5.manager.perClass(defaultfalse). Whentrue,JUnitJupiterTestClassLifecycleManagercaches its adaptor under a namespace keyed by the test class, so each class gets its ownTestRunnerAdaptorand underlyingManager.BeforeSuite/AfterSuitethen fire once per class, the only sensible semantics when classes truly run in parallel. Whenfalse(the default), the manager is the classic JVM-wide singleton, matching pre-PR behaviour; this preserves compatibility with shared managed containers (WildFly, Payara, GlassFish) that boot once and serve every test class. Documented injunit5/README.adoc.Public API is unchanged.
Fixes #
Single-threaded behaviour unchanged. But running The Servlet TCK (1751 tests) now passes at parallelism=4 where it previously threw 80+ errors with Jetty 12.1.x. Execution going down from ~13 minutes to 2/3 minutes.
Summary by Sourcery
Make Arquillian core and JUnit 5 integration safe for parallel test class execution by using proper thread-local storage and thread-safe collections.
Enhancements:
Summary by Sourcery
Make Arquillian core and JUnit 5 integration safe for parallel test class execution by introducing an optional per-test-class manager and improving thread-safety primitives.
New Features:
Bug Fixes:
Enhancements:
Documentation: