Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@
import java.util.HashSet;
import java.util.List;
import java.util.Set;
import java.util.concurrent.CopyOnWriteArrayList;

import org.jboss.arquillian.container.spi.client.protocol.ProtocolDescription;
import org.jboss.shrinkwrap.api.Archive;

Expand All @@ -32,7 +34,7 @@ public class DeploymentScenario {
private final List<Deployment> deployments;

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.

}

public DeploymentScenario addDeployment(DeploymentDescription deployment) {
Expand Down Expand Up @@ -314,4 +316,4 @@ private void validateNotSameArchiveAndSameTarget(DeploymentDescription deploymen
}
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@
package org.jboss.arquillian.container.test.impl.client;

import java.lang.reflect.Method;
import java.util.Objects;

import org.jboss.arquillian.container.spi.Container;
import org.jboss.arquillian.container.spi.ContainerRegistry;
import org.jboss.arquillian.container.spi.client.deployment.Deployment;
Expand Down Expand Up @@ -136,12 +138,11 @@ private void createContext(EventContext<? extends TestEvent> context) {
* TODO: This should not rely on direct Reflection, but rather access the metadata through some
* common metadata layer.
*/

private void lookup(Method method, ResultCallback callback) {
DeploymentTargetDescription deploymentTarget = locateDeployment(method);

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.


Deployment deployment = deploymentScenario.deployment(deploymentTarget);
if (deployment == null && deploymentTarget != DeploymentTargetDescription.DEFAULT) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,9 @@
*/
package org.jboss.arquillian.core.impl;

import java.util.ArrayList;
import java.util.List;
import java.util.concurrent.CopyOnWriteArrayList;

import org.jboss.arquillian.core.spi.EventContext;
import org.jboss.arquillian.core.spi.InvocationException;
import org.jboss.arquillian.core.spi.NonManagedObserver;
Expand All @@ -32,8 +33,8 @@
*/
public class EventContextImpl<T> implements EventContext<T> {
private ManagerImpl manager;
private List<ObserverMethod> interceptors;
private List<ObserverMethod> observers;
private List<ObserverMethod> interceptors = new CopyOnWriteArrayList<>();
private List<ObserverMethod> observers = new CopyOnWriteArrayList<>();
private NonManagedObserver<T> nonManagedObserver;
private RuntimeLogger runtimeLogger;

Expand Down Expand Up @@ -70,8 +71,12 @@ public EventContextImpl(ManagerImpl manager, List<ObserverMethod> interceptors,
Validate.notNull(runtimeLogger, "Runtime logger must be specified");

this.manager = manager;
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);
}
Comment on lines -73 to +79
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.

this.nonManagedObserver = nonManagedObserver;
this.event = event;
this.runtimeLogger = runtimeLogger;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@
import java.util.List;
import java.util.Set;
import java.util.concurrent.Callable;
import java.util.concurrent.CopyOnWriteArrayList;

import org.jboss.arquillian.core.api.Injector;
import org.jboss.arquillian.core.api.annotation.ApplicationScoped;
import org.jboss.arquillian.core.api.event.ManagerStarted;
Expand Down Expand Up @@ -70,13 +72,13 @@ public class ManagerImpl implements Manager {
new ArquillianThreadLocal<Set<Class<? extends Throwable>>>() {
@Override
protected Set<Class<? extends Throwable>> initialValue() {
return new HashSet<Class<? extends Throwable>>();
return new HashSet<>();
}
};

ManagerImpl(final Collection<Class<? extends Context>> contextClasses, final Collection<Class<?>> extensionClasses) {
this.contexts = new ArrayList<Context>();
this.extensions = new ArrayList<Extension>();
this.contexts = new CopyOnWriteArrayList<>();
this.extensions = new CopyOnWriteArrayList<>();
this.runtimeLogger = new RuntimeLogger();

try {
Expand Down Expand Up @@ -129,7 +131,7 @@ public <T> void fire(T event, NonManagedObserver<T> nonManagedObserver) {
context.activate();
activatedApplicationContext = true;
}
new EventContextImpl<T>(this, interceptorObservers, observers, nonManagedObserver, event,
new EventContextImpl<>(this, interceptorObservers, observers, nonManagedObserver, event,
runtimeLogger).proceed();
} catch (Exception e) {
Throwable fireException = e;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,12 @@ public class JUnitJupiterTestClassLifecycleManager implements AutoCloseable,
ExtensionContext.Store.CloseableResource {
private static final String MANAGER_KEY = "testRunnerManager";

private TestRunnerAdaptor adaptor;
private volatile TestRunnerAdaptor adaptor;

private Throwable caughtInitializationException;

private JUnitJupiterTestClassLifecycleManager() {
private JUnitJupiterTestClassLifecycleManager() throws Exception {
initializeAdaptor();
}

static JUnitJupiterTestClassLifecycleManager getManager(ExtensionContext context) throws Exception {
Expand All @@ -40,7 +41,6 @@ static JUnitJupiterTestClassLifecycleManager getManager(ExtensionContext context
if (instance == null) {
instance = new JUnitJupiterTestClassLifecycleManager();
store.put(MANAGER_KEY, instance);
instance.initializeAdaptor();
}
// no, initialization has been attempted before and failed, refuse
// to do anything else
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,11 @@
*/
package org.jboss.arquillian.test.impl;

import java.util.HashMap;
import java.util.HashSet;
import java.util.Map;
import java.util.Set;
import java.util.concurrent.ConcurrentHashMap;

import org.jboss.arquillian.core.api.Instance;
import org.jboss.arquillian.core.api.InstanceProducer;
import org.jboss.arquillian.core.api.annotation.Inject;
Expand Down Expand Up @@ -59,7 +60,7 @@ public class TestContextHandler {

// 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.


public void createSuiteContext(@Observes(precedence = 100) EventContext<SuiteEvent> context) {
SuiteContext suiteContext = this.suiteContextInstance.get();
Expand Down