Skip to content

Conversation

@olamy
Copy link
Contributor

@olamy olamy commented Feb 4, 2025

  • investigate junit5 multi thread build failure Servlet TCK with Jetty
  • more thread safe collections
  • not sure about those but some thread safe fields

Short description of what this resolves:

Changes proposed in this pull request:

Fixes #

@rhusar
Copy link
Member

rhusar commented Feb 7, 2025

@olamy Can you link the reproducer?

@olamy
Copy link
Contributor Author

olamy commented Feb 8, 2025

@olamy Can you link the reproducer?

sure have a look here https://github.com/jetty-project/servlet-tck-run/ use branch jetty-12-ee11-parallel-test

mvn clean verify you may have a lot of download of snapshots, you will have to change the arquillian-core version in the pom. (it's using the version of this PR for testing purpose)

with the current content content of this PR I managed to reduced by a lot failure number because of this change https://github.com/arquillian/arquillian-core/pull/673/files#diff-5c09558098ba87d126edb03750d0b116404b1680a7821a0d206789d58f768d32R12

I still have some errors such

java.lang.NullPointerException: deploymentScenario cannot be null
	at java.base/java.util.Objects.requireNonNull(Objects.java:235)
	at org.jboss.arquillian.container.test.impl.client.ContainerEventController.lookup(ContainerEventController.java:147)
	at org.jboss.arquillian.container.test.impl.client.ContainerEventController.createContext(ContainerEventController.java:133)
	at org.jboss.arquillian.container.test.impl.client.ContainerEventController.createBeforeContext(ContainerEventController.java:117)

note I have added the check here https://github.com/arquillian/arquillian-core/pull/673/files#diff-94ece6fcd6683e6db69d1647fcf29bf2091d8f73deb8dd991848755b646e94d7R147 to track this.

ETA is some random state failures with each builds.

Update on reproducer:

get branch jetty-12-1-ee11 from https://github.com/jetty-project/servlet-tck-run.git
You will need to build locally branch jetty-12.1-fix-deployer from https://github.com/arquillian/arquillian-container-jetty

eventually update the property <arquillian.core.version>1.9.5.Final-SNAPSHOT</arquillian.core.version><arquillian.core.version>1.9.5.Final-SNAPSHOT</arquillian.core.version> from the sevlet-tck-run project

mvn clean verify -Djunit.jupiter.execution.parallel.enabled=true

@olamy olamy force-pushed the tck-multithread-failures branch 2 times, most recently from 05837e2 to b22f1d1 Compare April 4, 2025 01:19
@olamy olamy force-pushed the tck-multithread-failures branch from b22f1d1 to f323e49 Compare April 16, 2025 00:26
Copy link
Member

@jamezp jamezp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall I think this is a good idea to make Arquillian more thread-safe. However, I do think it will be quite a task to do so.

Comment on lines -73 to +79
this.interceptors = interceptors == null ? new ArrayList<ObserverMethod>() : interceptors;
this.observers = observers == null ? new ArrayList<ObserverMethod>() : observers;
if(interceptors!=null) {
this.interceptors.addAll(interceptors);
}
if(observers!=null) {
this.observers.addAll(observers);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should use the constructor instead of addAll() to avoid lock.

// Since there can be multiple AfterTestLifecycleEvents (After/AfterRules)
// and we don't know which is the last one, perform the clean up in AfterClass.
private Map<Class<?>, Set<Object>> activatedTestContexts = new HashMap<Class<?>, Set<Object>>();
private Map<Class<?>, Set<Object>> activatedTestContexts = new ConcurrentHashMap<>();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to already be guarded by itself. I don't think we need to change the type.


ContainerRegistry containerRegistry = this.containerRegistry.get();
DeploymentScenario deploymentScenario = this.deploymentScenario.get();
DeploymentScenario deploymentScenario = Objects.requireNonNull(this.deploymentScenario.get(), "deploymentScenario cannot be null");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what we gain here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not much I was using this as a marker as the main issue I have with parallel tests is the NPE here

Copy link
Member

@jamezp jamezp Apr 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah okay. I would think then that there is likely an issue activating the context in a multi-threaded environment. That's typically when null would be returned there. IIRC the context is a ThreadLocal map of some sort.


public DeploymentScenario() {
this.deployments = new ArrayList<Deployment>();
this.deployments = new CopyOnWriteArrayList<>();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More is likely needed here to really be thread-safe. Especially in the findDefaultDeployment method.

@olamy
Copy link
Contributor Author

olamy commented Apr 17, 2025

Overall I think this is a good idea to make Arquillian more thread-safe. However, I do think it will be quite a task to do so.

Yup I agree :)
I don't have a deep knowledge of the internal of core so I have just started small steps at least there is an easy way to reproduce the problem.

@rhusar rhusar added the hold label May 22, 2025
@starksm64
Copy link
Member

The core layer with its single thread event dispatch is inherently not threadsafe. The same for the container layer. We will have to target this in the 2.0 release when we look at how the runners introduce parallel tests in the lifecycle and move away from the event dispatch approach.

@olamy
Copy link
Contributor Author

olamy commented Jun 6, 2025

oh yup sure. Sorry I didn't reallly look deeply into it but definitely sounds like more big changes to ArrayList to CopyOnWriteArrayList

olamy added 4 commits July 29, 2025 09:48
Signed-off-by: Olivier Lamy <olamy@apache.org>
Signed-off-by: Olivier Lamy <olamy@apache.org>
Signed-off-by: Olivier Lamy <olamy@apache.org>
Signed-off-by: Olivier Lamy <olamy@apache.org>
@olamy olamy force-pushed the tck-multithread-failures branch from f323e49 to 2689ad8 Compare July 28, 2025 23:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants