Fix deadlock in NamespacedHierarchicalStore.computeIfAbsent() (#5348)

In #5231 evaluation of the default value creator was deferred until
after an entry was created in the concurrent hash map backing the store.
This enables recursive updates.  However, by calling `evaluateIfNotNull`
inside `compute` threads that lost the race would wait inside
`Map::compute` for the value to be evaluated.

Waiting here is a problem when the backing concurrent hashmap is
reaching capacity. The thread waiting inside compute holds a lock while
at the same time the thread that won the race will try to acquire a lock
to resize the map.

By making the threads that lost the race wait outside of `Map::compute`
we can prevent this dead lock. This does require that each thread
retries when another thread failed to insert a value. But eventually
either one other thread either successfully inserts a value or the
thread uses its own default value creator.

Fixes: #5346

Co-authored-by: Marc Philipp <mail@marcphilipp.de>

(cherry picked from commit f29de38)

Author: mpkorstanje

documentation/modules/ROOT/partials/release-notes/release-notes-6.0.3.adoc

@@ -16,7 +16,8 @@ repository on GitHub.
 [[v6.0.3-junit-platform-bug-fixes]]
 ==== Bug Fixes
 
-* ❓
+* A deadlock issue in `NamespacedHierarchicalStore.computeIfAbsent(N, K, Function)` has
+  been fixed.
 
 [[v6.0.3-junit-platform-deprecations-and-breaking-changes]]
 ==== Deprecations and Breaking Changes

junit-platform-engine/src/main/java/org/junit/platform/engine/support/store/NamespacedHierarchicalStore.java

@@ -223,22 +223,39 @@ public void close() {
 			rejectIfClosed();
 			return defaultCreator.apply(key);
 		});
-		var storedValue = storedValues.compute(compositeKey, //
-			(__, oldStoredValue) -> {
-				// guard against race conditions, repeated from getStoredValue
-				// this filters out failures inserted by computeIfAbsent
-				if (StoredValue.isNonNullAndPresent(oldStoredValue)) {
-					return oldStoredValue;
-				}
-				rejectIfClosed();
-				return candidateStoredValue;
-			});
 
-		// Only the caller that created the candidateStoredValue may run it
-		if (candidateStoredValue.equals(storedValue)) {
-			return candidateStoredValue.execute();
+		for (;;) {
+			var storedValue = storedValues.compute(compositeKey, //
+				(__, oldStoredValue) -> {
+					// The old stored value remains if a) there is an old stored value and
+					// b) the old stored value has not yet been evaluated or c) the old
+					// stored value was evaluated to a present value.
+					//
+					// Condition b ensures that we do not evaluate or await the evaluation
+					// inside `compute`, this would lead to recursive updates or deadlocks
+					// respectively.
+					// Condition c guards against race conditions (repeated from
+					// getStoredValue) this filters out failures inserted by
+					// computeIfAbsent.
+					if (oldStoredValue != null && (!oldStoredValue.isDone() || oldStoredValue.isPresent())) {
+						return oldStoredValue;
+					}
+					rejectIfClosed();
+					return candidateStoredValue;
+				});
+
+			// Only the caller that created the candidateStoredValue may run it
+			if (candidateStoredValue.equals(storedValue)) {
+				return candidateStoredValue.execute();
+			}
+			// Implicitly awaits evaluation and uses the result if it was present.
+			// this filters out failures inserted by computeIfAbsent
+			// Otherwise, another thread won the race but failed to insert a
+			// present value so we try again.
+			if (storedValue.isPresent()) {
+				return storedValue.evaluate();
+			}
 		}
-		return storedValue.evaluate();
 	}
 
 	/**
@@ -269,29 +286,44 @@ public <K, V> Object computeIfAbsent(N namespace, K key, Function<? super K, ? e
 			rejectIfClosed();
 			return Preconditions.notNull(defaultCreator.apply(key), "defaultCreator must not return null");
 		});
-		var storedValue = storedValues.compute(compositeKey, (__, oldStoredValue) -> {
-			// guard against race conditions
-			// computeIfAbsent replaces both null and absent values
-			if (StoredValue.evaluateIfNotNull(oldStoredValue) != null) {
-				return oldStoredValue;
+
+		for (;;) {
+			var storedValue = storedValues.compute(compositeKey, (__, oldStoredValue) -> {
+				// The old stored value remains if a) there is an old stored value and
+				// b) the old stored value has not yet been evaluated or c) the old
+				// stored value evaluated to null.
+				//
+				// Condition b ensures that we do not evaluate or await the evaluation
+				// inside `compute`, this would lead to recursive updates or deadlocks
+				// respectively.
+				// Condition c ensures we replace both null and absent values.
+				if (oldStoredValue != null && (!oldStoredValue.isDone() || oldStoredValue.evaluate() != null)) {
+					return oldStoredValue;
+				}
+				rejectIfClosed();
+				return candidateStoredValue;
+			});
+
+			// Only the caller that created the candidateStoredValue may run it
+			// and see the exception.
+			if (candidateStoredValue.equals(storedValue)) {
+				Object newResult = candidateStoredValue.execute();
+				// DeferredOptionalValue is quite heavy, replace with lighter container
+				if (candidateStoredValue.isPresent()) {
+					storedValues.computeIfPresent(compositeKey, compareAndPut(storedValue, newStoredValue(newResult)));
+				}
+				return newResult;
 			}
-			rejectIfClosed();
-			return candidateStoredValue;
-		});
 
-		// In a race condition either put, getOrComputeIfAbsent, or another
-		// computeIfAbsent call put a non-null value in the store
-		if (!candidateStoredValue.equals(storedValue)) {
-			return requireNonNull(storedValue.evaluate());
-		}
-		// Only the caller that created the candidateStoredValue may run it
-		// and see the exception.
-		Object newResult = candidateStoredValue.execute();
-		// DeferredOptionalValue is quite heavy, replace with lighter container
-		if (candidateStoredValue.isPresent()) {
-			storedValues.computeIfPresent(compositeKey, compareAndPut(storedValue, newStoredValue(newResult)));
+			// Awaits evaluation and uses the stored value if it was not null
+			// this ensures we replace both null and absent values.
+			// Otherwise, another thread won the race but failed to insert a
+			// non-null value so we try again.
+			Object storedResult = storedValue.evaluate();
+			if (storedResult != null) {
+				return storedResult;
+			}
 		}
-		return newResult;
 	}
 
 	private static <N> BiFunction<CompositeKey<N>, StoredValue, StoredValue> compareAndPut(StoredValue expectedValue,
@@ -488,6 +520,8 @@ private interface StoredValue {
 
 		boolean isPresent();
 
+		boolean isDone();
+
 		static @Nullable Object evaluateIfNotNull(@Nullable StoredValue value) {
 			return value != null ? value.evaluate() : null;
 		}
@@ -518,6 +552,11 @@ public boolean isPresent() {
 				return true;
 			}
 
+			@Override
+			public boolean isDone() {
+				return true;
+			}
+
 			@Override
 			public int order() {
 				return order;
@@ -546,6 +585,11 @@ public boolean isPresent() {
 				return true;
 			}
 
+			@Override
+			public boolean isDone() {
+				return delegate.isDone();
+			}
+
 			@Nullable
 			Object execute() {
 				delegate.run();
@@ -580,6 +624,11 @@ public boolean isPresent() {
 				return evaluate() != null;
 			}
 
+			@Override
+			public boolean isDone() {
+				return delegate.isDone();
+			}
+
 			Object execute() {
 				delegate.run();
 				// Delegate does not produce null
@@ -670,6 +719,10 @@ Object getOrThrow() {
 				throw throwAsUncheckedException(cause);
 			}
 		}
+
+		boolean isDone() {
+			return task.isDone();
+		}
 	}
 
 	/**

platform-tests/src/test/java/org/junit/platform/engine/support/store/NamespacedHierarchicalStoreTests.java

@@ -33,6 +33,7 @@
 
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Nested;
+import org.junit.jupiter.api.RepeatedTest;
 import org.junit.jupiter.api.Test;
 
 /**
@@ -431,6 +432,30 @@ void simulateRaceConditionInGetOrComputeIfAbsent() throws Exception {
 			assertThat(values).hasSize(threads).containsOnly(1);
 		}
 
+		@SuppressWarnings("deprecation")
+		@RepeatedTest(value = 10, failureThreshold = 1)
+		void simulateRaceConditionInGetOrComputeIfAbsentWithResizingMap() throws Exception {
+			int threads = 10;
+			AtomicInteger counter = new AtomicInteger();
+			List<Object> values;
+
+			try (var localStore = new NamespacedHierarchicalStore<>(null)) {
+				values = executeConcurrently(threads, //
+					() -> {
+						// Simulate other extensions computing values,
+						// this will trigger several resizes of the store
+						for (int i = 0; i < 16; i++) {
+							localStore.getOrComputeIfAbsent(namespace, i, __ -> value);
+						}
+						return requireNonNull(
+							localStore.getOrComputeIfAbsent(namespace, key, it -> counter.incrementAndGet()));
+					});
+			}
+
+			assertEquals(1, counter.get());
+			assertThat(values).hasSize(threads).containsOnly(1);
+		}
+
 		@Test
 		void simulateRaceConditionInComputeIfAbsent() throws Exception {
 			int threads = 10;
@@ -446,6 +471,29 @@ void simulateRaceConditionInComputeIfAbsent() throws Exception {
 			assertThat(values).hasSize(threads).containsOnly(1);
 		}
 
+		@RepeatedTest(value = 10, failureThreshold = 1)
+		void simulateRaceConditionInComputeIfAbsentWithResizingMap() throws Exception {
+			int threads = 10;
+			AtomicInteger counter = new AtomicInteger();
+			List<Object> values;
+
+			try (var localStore = new NamespacedHierarchicalStore<>(null)) {
+				values = executeConcurrently(threads, //
+					() -> {
+						// Simulate other extensions computing values,
+						// this will trigger several resizes of the store
+						for (int i = 0; i < 16; i++) {
+							localStore.computeIfAbsent(namespace, i, s -> value);
+						}
+						return requireNonNull(
+							localStore.computeIfAbsent(namespace, key, it -> counter.incrementAndGet()));
+					});
+			}
+
+			assertEquals(1, counter.get());
+			assertThat(values).hasSize(threads).containsOnly(1);
+		}
+
 		@SuppressWarnings("deprecation")
 		@Test
 		void updateRecursivelyGetOrComputeIfAbsent() {