From 88f88591abc9900a3e30f0c3a9fca524b5c0f8b5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Leonard=20Br=C3=BCnings?= Date: Tue, 17 Sep 2024 10:58:05 +0200 Subject: [PATCH] Fix deadlock in `ForkJoinPoolHierarchicalTestExecutorService` (#3981) The service now checks if the `ExclusiveTask` that should run is executed on a thread that is already executing another task. If this is scenario is detected, it checks if the lock is compatible to the enclosing locks. 1. If compatible, it is executed and marked done 2. If incompatible, it is added to a list of deferred tasks and left unfinished. The deferred tasks will be re-forked afterwards. Fixes #3945. --------- Co-authored-by: Marc Philipp --- .../support/hierarchical/CompositeLock.java | 4 +- ...inPoolHierarchicalTestExecutorService.java | 110 ++++++++++- .../support/hierarchical/LockManager.java | 42 +++- .../support/hierarchical/NodeTestTask.java | 5 + .../support/hierarchical/ResourceLock.java | 8 + .../support/hierarchical/SingleLock.java | 16 ++ .../hierarchical/ForkJoinDeadLockTests.java | 179 ++++++++++++++++++ ...lHierarchicalTestExecutorServiceTests.java | 97 ++++++++++ .../hierarchical/LockManagerTests.java | 28 ++- .../hierarchical/ResourceLockTests.java | 82 ++++++++ 10 files changed, 549 insertions(+), 22 deletions(-) create mode 100644 platform-tests/src/test/java/org/junit/platform/engine/support/hierarchical/ForkJoinDeadLockTests.java create mode 100644 platform-tests/src/test/java/org/junit/platform/engine/support/hierarchical/ResourceLockTests.java diff --git a/junit-platform-engine/src/main/java/org/junit/platform/engine/support/hierarchical/CompositeLock.java b/junit-platform-engine/src/main/java/org/junit/platform/engine/support/hierarchical/CompositeLock.java index 3718d82a64fc..b09f1236f570 100644 --- a/junit-platform-engine/src/main/java/org/junit/platform/engine/support/hierarchical/CompositeLock.java +++ b/junit-platform-engine/src/main/java/org/junit/platform/engine/support/hierarchical/CompositeLock.java @@ -15,6 +15,8 @@ import java.util.concurrent.ForkJoinPool; import java.util.concurrent.locks.Lock; +import org.junit.platform.commons.util.Preconditions; + /** * @since 1.3 */ @@ -23,7 +25,7 @@ class CompositeLock implements ResourceLock { private final List locks; CompositeLock(List locks) { - this.locks = locks; + this.locks = Preconditions.notEmpty(locks, "Locks must not be empty"); } // for tests only diff --git a/junit-platform-engine/src/main/java/org/junit/platform/engine/support/hierarchical/ForkJoinPoolHierarchicalTestExecutorService.java b/junit-platform-engine/src/main/java/org/junit/platform/engine/support/hierarchical/ForkJoinPoolHierarchicalTestExecutorService.java index 09afd8df3eb7..9b5d2efea14c 100644 --- a/junit-platform-engine/src/main/java/org/junit/platform/engine/support/hierarchical/ForkJoinPoolHierarchicalTestExecutorService.java +++ b/junit-platform-engine/src/main/java/org/junit/platform/engine/support/hierarchical/ForkJoinPoolHierarchicalTestExecutorService.java @@ -16,6 +16,8 @@ import java.lang.Thread.UncaughtExceptionHandler; import java.lang.reflect.Constructor; +import java.util.ArrayDeque; +import java.util.ArrayList; import java.util.Deque; import java.util.LinkedList; import java.util.List; @@ -26,7 +28,6 @@ import java.util.concurrent.ForkJoinTask; import java.util.concurrent.ForkJoinWorkerThread; import java.util.concurrent.Future; -import java.util.concurrent.RecursiveAction; import java.util.concurrent.TimeUnit; import java.util.function.Function; import java.util.function.Predicate; @@ -51,7 +52,9 @@ public class ForkJoinPoolHierarchicalTestExecutorService implements HierarchicalTestExecutorService { private final ForkJoinPool forkJoinPool; + private final TaskEventListener taskEventListener; private final int parallelism; + private final ThreadLocal threadLocks = ThreadLocal.withInitial(ThreadLock::new); /** * Create a new {@code ForkJoinPoolHierarchicalTestExecutorService} based on @@ -71,7 +74,13 @@ public ForkJoinPoolHierarchicalTestExecutorService(ConfigurationParameters confi */ @API(status = STABLE, since = "1.10") public ForkJoinPoolHierarchicalTestExecutorService(ParallelExecutionConfiguration configuration) { + this(configuration, TaskEventListener.NOOP); + } + + ForkJoinPoolHierarchicalTestExecutorService(ParallelExecutionConfiguration configuration, + TaskEventListener taskEventListener) { forkJoinPool = createForkJoinPool(configuration); + this.taskEventListener = taskEventListener; parallelism = forkJoinPool.getParallelism(); LoggerFactory.getLogger(getClass()).config(() -> "Using ForkJoinPool with parallelism of " + parallelism); } @@ -132,7 +141,7 @@ public Future submit(TestTask testTask) { if (testTask.getExecutionMode() == CONCURRENT && ForkJoinTask.getSurplusQueuedTaskCount() < parallelism) { return exclusiveTask.fork(); } - exclusiveTask.compute(); + exclusiveTask.execSync(); return completedFuture(null); } @@ -143,7 +152,7 @@ private boolean isAlreadyRunningInForkJoinPool() { @Override public void invokeAll(List tasks) { if (tasks.size() == 1) { - new ExclusiveTask(tasks.get(0)).compute(); + new ExclusiveTask(tasks.get(0)).execSync(); return; } Deque nonConcurrentTasks = new LinkedList<>(); @@ -169,7 +178,7 @@ private void forkConcurrentTasks(List tasks, Deque nonConcurrentTasks) { for (ExclusiveTask task : nonConcurrentTasks) { - task.compute(); + task.execSync(); } } @@ -177,7 +186,18 @@ private void joinConcurrentTasksInReverseOrderToEnableWorkStealing( Deque concurrentTasksInReverseOrder) { for (ExclusiveTask forkedTask : concurrentTasksInReverseOrder) { forkedTask.join(); + resubmitDeferredTasks(); + } + } + + private void resubmitDeferredTasks() { + List deferredTasks = threadLocks.get().deferredTasks; + for (ExclusiveTask deferredTask : deferredTasks) { + if (!deferredTask.isDone()) { + deferredTask.fork(); + } } + deferredTasks.clear(); } @Override @@ -186,8 +206,8 @@ public void close() { } // this class cannot not be serialized because TestTask is not Serializable - @SuppressWarnings("serial") - static class ExclusiveTask extends RecursiveAction { + @SuppressWarnings({ "serial", "RedundantSuppression" }) + class ExclusiveTask extends ForkJoinTask { private final TestTask testTask; @@ -195,17 +215,61 @@ static class ExclusiveTask extends RecursiveAction { this.testTask = testTask; } + /** + * Always returns {@code null}. + * + * @return {@code null} always + */ + public final Void getRawResult() { + return null; + } + + /** + * Requires null completion value. + */ + protected final void setRawResult(Void mustBeNull) { + } + + void execSync() { + boolean completed = exec(); + if (!completed) { + throw new IllegalStateException( + "Task was deferred but should have been executed synchronously: " + testTask); + } + } + @SuppressWarnings("try") @Override - public void compute() { - try (ResourceLock lock = testTask.getResourceLock().acquire()) { + public boolean exec() { + // Check if this task is compatible with the current resource lock, if there is any. + // If not, we put this task in the thread local as a deferred task + // and let the worker thread fork it once it is done with the current task. + ResourceLock resourceLock = testTask.getResourceLock(); + ThreadLock threadLock = threadLocks.get(); + if (!threadLock.areAllHeldLocksCompatibleWith(resourceLock)) { + threadLock.addDeferredTask(this); + taskEventListener.deferred(testTask); + // Return false to indicate that this task is not done yet + // this means that .join() will wait. + return false; + } + try (ResourceLock lock = resourceLock.acquire()) { + threadLock.incrementNesting(lock); testTask.execute(); + return true; } catch (InterruptedException e) { throw ExceptionUtils.throwAsUncheckedException(e); } + finally { + threadLock.decrementNesting(); + } } + @Override + public String toString() { + return "ExclusiveTask [" + testTask + "]"; + } } static class WorkerThreadFactory implements ForkJoinPool.ForkJoinWorkerThreadFactory { @@ -228,4 +292,34 @@ static class WorkerThread extends ForkJoinWorkerThread { } + static class ThreadLock { + private final Deque locks = new ArrayDeque<>(2); + private final List deferredTasks = new ArrayList<>(); + + void addDeferredTask(ExclusiveTask task) { + deferredTasks.add(task); + } + + void incrementNesting(ResourceLock lock) { + locks.push(lock); + } + + @SuppressWarnings("resource") + void decrementNesting() { + locks.pop(); + } + + boolean areAllHeldLocksCompatibleWith(ResourceLock lock) { + return locks.stream().allMatch(l -> l.isCompatible(lock)); + } + } + + interface TaskEventListener { + + TaskEventListener NOOP = __ -> { + }; + + void deferred(TestTask testTask); + } + } diff --git a/junit-platform-engine/src/main/java/org/junit/platform/engine/support/hierarchical/LockManager.java b/junit-platform-engine/src/main/java/org/junit/platform/engine/support/hierarchical/LockManager.java index 223f7873525d..be997dc5715a 100644 --- a/junit-platform-engine/src/main/java/org/junit/platform/engine/support/hierarchical/LockManager.java +++ b/junit-platform-engine/src/main/java/org/junit/platform/engine/support/hierarchical/LockManager.java @@ -10,12 +10,16 @@ package org.junit.platform.engine.support.hierarchical; +import static java.util.Collections.emptyList; +import static java.util.Collections.singletonList; import static java.util.Comparator.comparing; import static java.util.Comparator.naturalOrder; import static java.util.stream.Collectors.groupingBy; import static java.util.stream.Collectors.toList; import static org.junit.platform.commons.util.CollectionUtils.getOnlyElement; import static org.junit.platform.engine.support.hierarchical.ExclusiveResource.GLOBAL_KEY; +import static org.junit.platform.engine.support.hierarchical.ExclusiveResource.GLOBAL_READ; +import static org.junit.platform.engine.support.hierarchical.ExclusiveResource.GLOBAL_READ_WRITE; import static org.junit.platform.engine.support.hierarchical.ExclusiveResource.LockMode.READ; import java.util.Collection; @@ -28,6 +32,9 @@ import java.util.concurrent.locks.ReadWriteLock; import java.util.concurrent.locks.ReentrantReadWriteLock; +import org.junit.platform.engine.support.hierarchical.SingleLock.GlobalReadLock; +import org.junit.platform.engine.support.hierarchical.SingleLock.GlobalReadWriteLock; + /** * @since 1.3 */ @@ -42,20 +49,29 @@ private static Comparator globalKeyFirst() { } private final Map locksByKey = new ConcurrentHashMap<>(); + private final GlobalReadLock globalReadLock; + private final GlobalReadWriteLock globalReadWriteLock; + + public LockManager() { + globalReadLock = new GlobalReadLock(toLock(GLOBAL_READ)); + globalReadWriteLock = new GlobalReadWriteLock(toLock(GLOBAL_READ_WRITE)); + } ResourceLock getLockForResources(Collection resources) { - if (resources.size() == 1) { - return getLockForResource(getOnlyElement(resources)); - } - List locks = getDistinctSortedLocks(resources); - return toResourceLock(locks); + return toResourceLock(toDistinctSortedLocks(resources)); } ResourceLock getLockForResource(ExclusiveResource resource) { - return new SingleLock(toLock(resource)); + return toResourceLock(toLock(resource)); } - private List getDistinctSortedLocks(Collection resources) { + private List toDistinctSortedLocks(Collection resources) { + if (resources.isEmpty()) { + return emptyList(); + } + if (resources.size() == 1) { + return singletonList(toLock(getOnlyElement(resources))); + } // @formatter:off Map> resourcesByKey = resources.stream() .sorted(COMPARATOR) @@ -79,10 +95,20 @@ private ResourceLock toResourceLock(List locks) { case 0: return NopLock.INSTANCE; case 1: - return new SingleLock(locks.get(0)); + return toResourceLock(locks.get(0)); default: return new CompositeLock(locks); } } + private ResourceLock toResourceLock(Lock lock) { + if (lock == toLock(GLOBAL_READ)) { + return globalReadLock; + } + if (lock == toLock(GLOBAL_READ_WRITE)) { + return globalReadWriteLock; + } + return new SingleLock(lock); + } + } diff --git a/junit-platform-engine/src/main/java/org/junit/platform/engine/support/hierarchical/NodeTestTask.java b/junit-platform-engine/src/main/java/org/junit/platform/engine/support/hierarchical/NodeTestTask.java index e4d2208f2bc3..8e602b990ba9 100644 --- a/junit-platform-engine/src/main/java/org/junit/platform/engine/support/hierarchical/NodeTestTask.java +++ b/junit-platform-engine/src/main/java/org/junit/platform/engine/support/hierarchical/NodeTestTask.java @@ -79,6 +79,11 @@ public ExecutionMode getExecutionMode() { return taskContext.getExecutionAdvisor().getForcedExecutionMode(testDescriptor).orElse(node.getExecutionMode()); } + @Override + public String toString() { + return "NodeTestTask [" + testDescriptor + "]"; + } + void setParentContext(C parentContext) { this.parentContext = parentContext; } diff --git a/junit-platform-engine/src/main/java/org/junit/platform/engine/support/hierarchical/ResourceLock.java b/junit-platform-engine/src/main/java/org/junit/platform/engine/support/hierarchical/ResourceLock.java index 1dcdb5601881..19f6b4da061b 100644 --- a/junit-platform-engine/src/main/java/org/junit/platform/engine/support/hierarchical/ResourceLock.java +++ b/junit-platform-engine/src/main/java/org/junit/platform/engine/support/hierarchical/ResourceLock.java @@ -43,4 +43,12 @@ default void close() { release(); } + /** + * {@return whether the given lock is compatible with this lock} + * @param other the other lock to check for compatibility + */ + default boolean isCompatible(ResourceLock other) { + return this instanceof NopLock || other instanceof NopLock; + } + } diff --git a/junit-platform-engine/src/main/java/org/junit/platform/engine/support/hierarchical/SingleLock.java b/junit-platform-engine/src/main/java/org/junit/platform/engine/support/hierarchical/SingleLock.java index e568b9a8f5c0..f3853dce4ee8 100644 --- a/junit-platform-engine/src/main/java/org/junit/platform/engine/support/hierarchical/SingleLock.java +++ b/junit-platform-engine/src/main/java/org/junit/platform/engine/support/hierarchical/SingleLock.java @@ -60,4 +60,20 @@ public boolean isReleasable() { } + static class GlobalReadLock extends SingleLock { + GlobalReadLock(Lock lock) { + super(lock); + } + + @Override + public boolean isCompatible(ResourceLock other) { + return !(other instanceof GlobalReadWriteLock); + } + } + + static class GlobalReadWriteLock extends SingleLock { + GlobalReadWriteLock(Lock lock) { + super(lock); + } + } } diff --git a/platform-tests/src/test/java/org/junit/platform/engine/support/hierarchical/ForkJoinDeadLockTests.java b/platform-tests/src/test/java/org/junit/platform/engine/support/hierarchical/ForkJoinDeadLockTests.java new file mode 100644 index 000000000000..b50b2e23c700 --- /dev/null +++ b/platform-tests/src/test/java/org/junit/platform/engine/support/hierarchical/ForkJoinDeadLockTests.java @@ -0,0 +1,179 @@ +/* + * Copyright 2015-2024 the original author or authors. + * + * All rights reserved. This program and the accompanying materials are + * made available under the terms of the Eclipse Public License v2.0 which + * accompanies this distribution and is available at + * + * https://www.eclipse.org/legal/epl-v20.html + */ + +package org.junit.platform.engine.support.hierarchical; + +import static org.junit.jupiter.api.parallel.ExecutionMode.CONCURRENT; +import static org.junit.jupiter.api.parallel.ExecutionMode.SAME_THREAD; +import static org.junit.jupiter.api.parallel.ResourceAccessMode.READ; +import static org.junit.jupiter.api.parallel.ResourceAccessMode.READ_WRITE; +import static org.junit.jupiter.api.parallel.Resources.SYSTEM_PROPERTIES; + +import java.time.LocalTime; +import java.util.Arrays; +import java.util.concurrent.CountDownLatch; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.Timeout; +import org.junit.jupiter.api.extension.AfterAllCallback; +import org.junit.jupiter.api.extension.AfterTestExecutionCallback; +import org.junit.jupiter.api.extension.BeforeAllCallback; +import org.junit.jupiter.api.extension.BeforeTestExecutionCallback; +import org.junit.jupiter.api.extension.ExtendWith; +import org.junit.jupiter.api.extension.ExtensionContext; +import org.junit.jupiter.api.parallel.Execution; +import org.junit.jupiter.api.parallel.Isolated; +import org.junit.jupiter.api.parallel.ResourceLock; +import org.junit.jupiter.engine.Constants; +import org.junit.platform.engine.discovery.ClassSelector; +import org.junit.platform.engine.discovery.DiscoverySelectors; +import org.junit.platform.testkit.engine.EngineTestKit; + +// https://github.com/junit-team/junit5/issues/3945 +@Timeout(10) +public class ForkJoinDeadLockTests { + + @Test + void forkJoinExecutionDoesNotLeadToDeadLock() { + run(NonIsolatedTestCase.class, IsolatedTestCase.class, Isolated2TestCase.class); + } + + @Test + void nestedResourceLocksShouldStillWork() { + run(SharedResourceTestCase.class); + } + + @Test + void multiLevelLocks() { + run(ClassLevelTestCase.class); + } + + private static void run(Class... classes) { + EngineTestKit.engine("junit-jupiter") // + .selectors(Arrays.stream(classes).map(DiscoverySelectors::selectClass).toArray(ClassSelector[]::new)) // + .configurationParameter(Constants.PARALLEL_EXECUTION_ENABLED_PROPERTY_NAME, "true") // + .configurationParameter(Constants.DEFAULT_PARALLEL_EXECUTION_MODE, "concurrent") // + .configurationParameter(Constants.DEFAULT_CLASSES_EXECUTION_MODE_PROPERTY_NAME, "concurrent") // + .configurationParameter(Constants.PARALLEL_CONFIG_STRATEGY_PROPERTY_NAME, "fixed") // + .configurationParameter(Constants.PARALLEL_CONFIG_FIXED_MAX_POOL_SIZE_PROPERTY_NAME, "3") // + .configurationParameter(Constants.PARALLEL_CONFIG_FIXED_PARALLELISM_PROPERTY_NAME, "3") // + .configurationParameter(Constants.PARALLEL_CONFIG_FIXED_SATURATE_PROPERTY_NAME, "false") // + .execute(); + } + + @ExtendWith(StartFinishLogger.class) + static class BaseTestCase { + } + + @SuppressWarnings("JUnitMalformedDeclaration") + @Execution(CONCURRENT) + public static class NonIsolatedTestCase extends BaseTestCase { + + public static CountDownLatch otherThreadRunning = new CountDownLatch(1); + public static CountDownLatch sameThreadFinishing = new CountDownLatch(1); + + @Test + @Execution(CONCURRENT) + void otherThread() throws Exception { + otherThreadRunning.countDown(); + sameThreadFinishing.await(); + Thread.sleep(100); + } + + @Test + @Execution(SAME_THREAD) + void sameThread() throws Exception { + otherThreadRunning.await(); + sameThreadFinishing.countDown(); + } + } + + @SuppressWarnings("JUnitMalformedDeclaration") + @Isolated + public static class IsolatedTestCase extends BaseTestCase { + + @Test + void test() throws Exception { + Thread.sleep(100); + } + } + + static class Isolated2TestCase extends IsolatedTestCase { + } + + @SuppressWarnings("JUnitMalformedDeclaration") + public static class SharedResourceTestCase { + + @Test + @ResourceLock(value = SYSTEM_PROPERTIES, mode = READ) + void customPropertyIsNotSetByDefault() { + } + + @Test + @ResourceLock(value = SYSTEM_PROPERTIES, mode = READ_WRITE) + void canSetCustomPropertyToApple() { + } + + @Test + @ResourceLock(value = SYSTEM_PROPERTIES, mode = READ_WRITE) + void canSetCustomPropertyToBanana() { + } + } + + @SuppressWarnings("JUnitMalformedDeclaration") + @ResourceLock(value = "foo", mode = READ_WRITE) + public static class ClassLevelTestCase { + + @Test + @ResourceLock(value = SYSTEM_PROPERTIES, mode = READ) + void customPropertyIsNotSetByDefault() { + } + + @Test + @ResourceLock(value = SYSTEM_PROPERTIES, mode = READ_WRITE) + void canSetCustomPropertyToApple() { + } + + @Test + @ResourceLock(value = SYSTEM_PROPERTIES, mode = READ_WRITE) + void canSetCustomPropertyToBanana() { + } + } + + static class StartFinishLogger + implements BeforeTestExecutionCallback, AfterTestExecutionCallback, BeforeAllCallback, AfterAllCallback { + + @Override + public void beforeAll(ExtensionContext context) { + log("starting class " + context.getTestClass().orElseThrow().getSimpleName()); + } + + @Override + public void beforeTestExecution(ExtensionContext context) { + log("starting method " + context.getTestClass().orElseThrow().getSimpleName() + "." + + context.getTestMethod().orElseThrow().getName()); + } + + @Override + public void afterTestExecution(ExtensionContext context) { + log("finishing method " + context.getTestClass().orElseThrow().getSimpleName() + "." + + context.getTestMethod().orElseThrow().getName()); + } + + @Override + public void afterAll(ExtensionContext context) { + log("finishing class " + context.getTestClass().orElseThrow().getSimpleName()); + } + } + + private static void log(String message) { + System.out.println("[" + LocalTime.now() + "] " + Thread.currentThread().getName() + " - " + message); + } +} diff --git a/platform-tests/src/test/java/org/junit/platform/engine/support/hierarchical/ForkJoinPoolHierarchicalTestExecutorServiceTests.java b/platform-tests/src/test/java/org/junit/platform/engine/support/hierarchical/ForkJoinPoolHierarchicalTestExecutorServiceTests.java index 79120bf40d17..f3ee3d923ab6 100644 --- a/platform-tests/src/test/java/org/junit/platform/engine/support/hierarchical/ForkJoinPoolHierarchicalTestExecutorServiceTests.java +++ b/platform-tests/src/test/java/org/junit/platform/engine/support/hierarchical/ForkJoinPoolHierarchicalTestExecutorServiceTests.java @@ -11,11 +11,26 @@ package org.junit.platform.engine.support.hierarchical; import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.platform.engine.support.hierarchical.ExclusiveResource.GLOBAL_READ; +import static org.junit.platform.engine.support.hierarchical.ExclusiveResource.GLOBAL_READ_WRITE; +import static org.junit.platform.engine.support.hierarchical.Node.ExecutionMode.CONCURRENT; + +import java.util.List; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.atomic.AtomicReference; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.Timeout; +import org.junit.jupiter.api.function.ThrowingConsumer; import org.junit.platform.commons.JUnitException; +import org.junit.platform.engine.support.hierarchical.ForkJoinPoolHierarchicalTestExecutorService.TaskEventListener; +import org.junit.platform.engine.support.hierarchical.HierarchicalTestExecutorService.TestTask; +import org.junit.platform.engine.support.hierarchical.Node.ExecutionMode; class ForkJoinPoolHierarchicalTestExecutorServiceTests { @@ -33,4 +48,86 @@ void exceptionsFromInvalidConfigurationAreNotSwallowed() { assertThat(exception).rootCause().isInstanceOf(IllegalArgumentException.class); } + @Test + @Timeout(5) + void defersTasksWithIncompatibleLocks() throws Exception { + var configuration = new DefaultParallelExecutionConfiguration(2, 2, 2, 2, 1, __ -> true); + + var lockManager = new LockManager(); + var globalReadLock = lockManager.getLockForResource(GLOBAL_READ); + var globalReadWriteLock = lockManager.getLockForResource(GLOBAL_READ_WRITE); + var nopLock = NopLock.INSTANCE; + + var threadNamesByTaskIdentifier = new ConcurrentHashMap(); + var deferred = new CountDownLatch(1); + var deferredTask = new AtomicReference(); + + TaskEventListener taskEventListener = testTask -> { + deferredTask.set(testTask); + deferred.countDown(); + }; + + var isolatedTask = new DummyTestTask("isolatedTask", globalReadWriteLock, + t -> threadNamesByTaskIdentifier.put(t.identifier(), Thread.currentThread().getName())); + + try (var pool = new ForkJoinPoolHierarchicalTestExecutorService(configuration, taskEventListener)) { + + var bothLeafTasksAreRunning = new CountDownLatch(2); + var nestedTask = new DummyTestTask("nestedTask", globalReadLock, t -> { + threadNamesByTaskIdentifier.put(t.identifier(), Thread.currentThread().getName()); + var leafTask1 = new DummyTestTask("leafTask1", nopLock, t1 -> { + threadNamesByTaskIdentifier.put(t1.identifier(), Thread.currentThread().getName()); + pool.new ExclusiveTask(isolatedTask).fork(); + bothLeafTasksAreRunning.countDown(); + bothLeafTasksAreRunning.await(); + try { + deferred.await(); + } + catch (InterruptedException e) { + System.out.println("Interrupted while waiting for task to be deferred"); + } + }); + var leafTask2 = new DummyTestTask("leafTask2", nopLock, t2 -> { + threadNamesByTaskIdentifier.put(t2.identifier(), Thread.currentThread().getName()); + bothLeafTasksAreRunning.countDown(); + bothLeafTasksAreRunning.await(); + }); + pool.invokeAll(List.of(leafTask1, leafTask2)); + }); + + pool.submit(nestedTask).get(); + } + + assertEquals(isolatedTask, deferredTask.get()); + assertEquals(threadNamesByTaskIdentifier.get("nestedTask"), threadNamesByTaskIdentifier.get("leafTask2")); + assertNotEquals(threadNamesByTaskIdentifier.get("leafTask1"), threadNamesByTaskIdentifier.get("leafTask2")); + } + + record DummyTestTask(String identifier, ResourceLock resourceLock, ThrowingConsumer action) + implements TestTask { + @Override + public ExecutionMode getExecutionMode() { + return CONCURRENT; + } + + @Override + public ResourceLock getResourceLock() { + return resourceLock; + } + + @Override + public void execute() { + try { + action.accept(this); + } + catch (Throwable e) { + throw new RuntimeException("Action " + identifier + " failed", e); + } + } + + @Override + public String toString() { + return identifier; + } + } } diff --git a/platform-tests/src/test/java/org/junit/platform/engine/support/hierarchical/LockManagerTests.java b/platform-tests/src/test/java/org/junit/platform/engine/support/hierarchical/LockManagerTests.java index 5fca505147f1..c9cb0570027e 100644 --- a/platform-tests/src/test/java/org/junit/platform/engine/support/hierarchical/LockManagerTests.java +++ b/platform-tests/src/test/java/org/junit/platform/engine/support/hierarchical/LockManagerTests.java @@ -32,7 +32,7 @@ */ class LockManagerTests { - private LockManager lockManager = new LockManager(); + private final LockManager lockManager = new LockManager(); @Test void returnsNopLockWithoutExclusiveResources() { @@ -50,7 +50,7 @@ void returnsSingleLockForSingleExclusiveResource() { var locks = getLocks(resources, SingleLock.class); assertThat(locks).hasSize(1); - assertThat(locks.get(0)).isInstanceOf(ReadLock.class); + assertThat(locks.getFirst()).isInstanceOf(ReadLock.class); } @Test @@ -75,7 +75,7 @@ void reusesSameLockForExclusiveResourceWithSameKey() { assertThat(locks1).hasSize(1); assertThat(locks2).hasSize(1); - assertThat(locks1.get(0)).isSameAs(locks2.get(0)); + assertThat(locks1.getFirst()).isSameAs(locks2.getFirst()); } @Test @@ -111,8 +111,26 @@ void globalLockComesFirst(LockMode globalLockMode) { assertThat(locks.get(3)).isEqualTo(getSingleLock("foo", READ_WRITE)); } - private Lock getSingleLock(String globalResourceLockKey, LockMode read) { - return getLocks(Set.of(new ExclusiveResource(globalResourceLockKey, read)), SingleLock.class).get(0); + @Test + void usesSpecialClassForGlobalReadLock() { + var lock = lockManager.getLockForResources(List.of(ExclusiveResource.GLOBAL_READ)); + + assertThat(lock) // + .isInstanceOf(SingleLock.GlobalReadLock.class) // + .isSameAs(lockManager.getLockForResource(ExclusiveResource.GLOBAL_READ)); + } + + @Test + void usesSpecialClassForGlobalReadWriteLock() { + var lock = lockManager.getLockForResources(List.of(ExclusiveResource.GLOBAL_READ_WRITE)); + + assertThat(lock) // + .isInstanceOf(SingleLock.GlobalReadWriteLock.class) // + .isSameAs(lockManager.getLockForResource(ExclusiveResource.GLOBAL_READ_WRITE)); + } + + private Lock getSingleLock(String key, LockMode lockMode) { + return getLocks(Set.of(new ExclusiveResource(key, lockMode)), SingleLock.class).getFirst(); } private List getLocks(Collection resources, Class type) { diff --git a/platform-tests/src/test/java/org/junit/platform/engine/support/hierarchical/ResourceLockTests.java b/platform-tests/src/test/java/org/junit/platform/engine/support/hierarchical/ResourceLockTests.java new file mode 100644 index 000000000000..ca80c33d6131 --- /dev/null +++ b/platform-tests/src/test/java/org/junit/platform/engine/support/hierarchical/ResourceLockTests.java @@ -0,0 +1,82 @@ +/* + * Copyright 2015-2024 the original author or authors. + * + * All rights reserved. This program and the accompanying materials are + * made available under the terms of the Eclipse Public License v2.0 which + * accompanies this distribution and is available at + * + * https://www.eclipse.org/legal/epl-v20.html + */ + +package org.junit.platform.engine.support.hierarchical; + +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.util.List; +import java.util.concurrent.locks.ReentrantLock; + +import org.junit.jupiter.api.Test; + +@SuppressWarnings("resource") +class ResourceLockTests { + + @Test + void nopLocksAreCompatibleWithEverything() { + var nopLock = NopLock.INSTANCE; + + assertTrue(nopLock.isCompatible(NopLock.INSTANCE)); + assertTrue(nopLock.isCompatible(new SingleLock(anyReentrantLock()))); + assertTrue(nopLock.isCompatible(new SingleLock.GlobalReadLock(anyReentrantLock()))); + assertTrue(nopLock.isCompatible(new SingleLock.GlobalReadWriteLock(anyReentrantLock()))); + assertTrue(nopLock.isCompatible(new CompositeLock(List.of(anyReentrantLock())))); + } + + @Test + void singleLocksAreIncompatibleWithNonNopLocks() { + var singleLock = new SingleLock(anyReentrantLock()); + + assertTrue(singleLock.isCompatible(NopLock.INSTANCE)); + assertFalse(singleLock.isCompatible(new SingleLock(anyReentrantLock()))); + assertFalse(singleLock.isCompatible(new SingleLock.GlobalReadLock(anyReentrantLock()))); + assertFalse(singleLock.isCompatible(new SingleLock.GlobalReadWriteLock(anyReentrantLock()))); + assertFalse(singleLock.isCompatible(new CompositeLock(List.of(anyReentrantLock())))); + } + + @Test + void globalReadLockIsCompatibleWithEverythingExceptGlobalReadWriteLock() { + var globalReadLock = new SingleLock.GlobalReadLock(anyReentrantLock()); + + assertTrue(globalReadLock.isCompatible(NopLock.INSTANCE)); + assertTrue(globalReadLock.isCompatible(new SingleLock(anyReentrantLock()))); + assertTrue(globalReadLock.isCompatible(new SingleLock.GlobalReadLock(anyReentrantLock()))); + assertFalse(globalReadLock.isCompatible(new SingleLock.GlobalReadWriteLock(anyReentrantLock()))); + assertTrue(globalReadLock.isCompatible(new CompositeLock(List.of(anyReentrantLock())))); + } + + @Test + void globalReadWriteLockIsIncompatibleWithWithNonNopLocks() { + var globalReadWriteLock = new SingleLock.GlobalReadWriteLock(anyReentrantLock()); + + assertTrue(globalReadWriteLock.isCompatible(NopLock.INSTANCE)); + assertFalse(globalReadWriteLock.isCompatible(new SingleLock(anyReentrantLock()))); + assertFalse(globalReadWriteLock.isCompatible(new SingleLock.GlobalReadLock(anyReentrantLock()))); + assertFalse(globalReadWriteLock.isCompatible(new SingleLock.GlobalReadWriteLock(anyReentrantLock()))); + assertFalse(globalReadWriteLock.isCompatible(new CompositeLock(List.of(anyReentrantLock())))); + } + + @Test + void compositeLocksAreIncompatibleWithNonNopLocks() { + CompositeLock compositeLock = new CompositeLock(List.of(anyReentrantLock())); + + assertTrue(compositeLock.isCompatible(NopLock.INSTANCE)); + assertFalse(compositeLock.isCompatible(new SingleLock(anyReentrantLock()))); + assertFalse(compositeLock.isCompatible(new SingleLock.GlobalReadLock(anyReentrantLock()))); + assertFalse(compositeLock.isCompatible(new SingleLock.GlobalReadWriteLock(anyReentrantLock()))); + assertFalse(compositeLock.isCompatible(compositeLock)); + } + + private static ReentrantLock anyReentrantLock() { + return new ReentrantLock(); + } +}