Fix concurrency bug in NamespacedHierarchicalStore.computeIfAbsent

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

@@ -17,6 +17,14 @@ repository on GitHub.
 ==== Bug Fixes
 
 * Make `ConsoleLauncher` compatible with JDK 26 by avoiding final field mutations.
+* Fix a concurrency issue in `NamespacedHierarchicalStore.computeIfAbsent` where
+`defaultCreator` was executed while holding the internal map's bucket lock,
+causing threads accessing different keys in the same hash bucket to block each
+other during parallel test execution
+(see link:{junit-framework-repo}+/issues/5171+[issue #5171] and
+link:https://github.com/assertj/assertj/issues/1996[assertj/assertj#1996]).
+* `NamespacedHierarchicalStore.computeIfAbsent` no longer deadlocks when
+`defaultCreator` accesses other keys that collide in the same hash bucket.
 
 [[v6.0.2-junit-platform-deprecations-and-breaking-changes]]
 ==== Deprecations and Breaking Changes
@@ -36,7 +44,7 @@ repository on GitHub.
 ==== Bug Fixes
 
 * Allow using `@ResourceLock` on classes annotated with `@ClassTemplate` (or
-  `@ParameterizedClass`).
+`@ParameterizedClass`).
 
 [[v6.0.2-junit-jupiter-deprecations-and-breaking-changes]]
 ==== Deprecations and Breaking Changes

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

@@ -25,6 +25,8 @@
 import java.util.Optional;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.ConcurrentMap;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.FutureTask;
 import java.util.concurrent.atomic.AtomicInteger;
 import java.util.function.Function;
 import java.util.function.Supplier;
@@ -241,24 +243,78 @@ public void close() {
 	public <K, V> Object computeIfAbsent(N namespace, K key, Function<? super K, ? extends V> defaultCreator) {
 		Preconditions.notNull(defaultCreator, "defaultCreator must not be null");
 		CompositeKey<N> compositeKey = new CompositeKey<>(namespace, key);
-		StoredValue storedValue = getStoredValue(compositeKey);
-		var result = StoredValue.evaluateIfNotNull(storedValue);
-		if (result == null) {
-			StoredValue newStoredValue = this.storedValues.compute(compositeKey, (__, oldStoredValue) -> {
-				if (StoredValue.evaluateIfNotNull(oldStoredValue) == null) {
-					rejectIfClosed();
-					var computedValue = Preconditions.notNull(defaultCreator.apply(key),
-						"defaultCreator must not return null");
-					return newStoredValue(() -> {
-						rejectIfClosed();
-						return computedValue;
-					});
+
+		// CAS-retry loop: retry if another thread concurrently modifies the entry
+		for (;;) {
+			StoredValue localStoredValue = this.storedValues.get(compositeKey);
+			if (localStoredValue != null) {
+				Object localValue = evaluateForComputeIfAbsent(compositeKey, localStoredValue);
+				if (localValue != null) {
+					return localValue;
+				}
+
+				Object computed = computeAndInstall(compositeKey, localStoredValue, key, defaultCreator);
+				if (computed != null) {
+					return computed;
+				}
+				continue;
+			}
+
+			// No local mapping: consult parent first.
+			if (this.parentStore != null) {
+				StoredValue parentStoredValue = this.parentStore.getStoredValue(compositeKey);
+				Object parentValue = StoredValue.evaluateIfNotNull(parentStoredValue);
+				if (parentValue != null) {
+					return parentValue;
 				}
-				return oldStoredValue;
-			});
-			return requireNonNull(newStoredValue.evaluate());
+			}
+
+			Object computed = computeAndInstall(compositeKey, null, key, defaultCreator);
+			if (computed != null) {
+				return computed;
+			}
+		}
+	}
+
+	private @Nullable Object evaluateForComputeIfAbsent(CompositeKey<N> compositeKey, StoredValue storedValue) {
+		Supplier<@Nullable Object> supplier = storedValue.supplier();
+		if (supplier instanceof DeferredSupplier deferred) {
+			deferred.run();
+			try {
+				return deferred.getOrThrow();
+			}
+			catch (Throwable t) {
+				this.storedValues.remove(compositeKey, storedValue);
+				throw t;
+			}
+		}
+		return storedValue.evaluate();
+	}
+
+	private <K, V> @Nullable Object computeAndInstall(CompositeKey<N> compositeKey, @Nullable StoredValue expectedOld,
+			K key, Function<? super K, ? extends V> defaultCreator) {
+
+		var deferred = new DeferredSupplier(() -> {
+			rejectIfClosed();
+			return Preconditions.notNull(defaultCreator.apply(key), "defaultCreator must not return null");
+		});
+		StoredValue newStoredValue = newStoredValue(deferred);
+
+		boolean installed = (expectedOld == null ? this.storedValues.putIfAbsent(compositeKey, newStoredValue) == null
+				: this.storedValues.replace(compositeKey, expectedOld, newStoredValue));
+
+		if (!installed) {
+			return null;
+		}
+
+		deferred.run();
+		try {
+			return requireNonNull(deferred.getOrThrow());
+		}
+		catch (Throwable t) {
+			this.storedValues.remove(compositeKey, newStoredValue);
+			throw t;
 		}
-		return result;
 	}
 
 	/**
@@ -460,6 +516,66 @@ private void close(CloseAction<N> closeAction) throws Throwable {
 
 	}
 
+	/**
+	 * Deferred computation that can be installed into the store without executing
+	 * user code while holding internal map locks.
+	 *
+	 * <p>For {@link #get(Object, Object)}, failures are treated as logically absent
+	 * (returning {@code null}) so exceptions are not observable via {@code get()}.
+	 *
+	 * <p>For {@link #computeIfAbsent(Object, Object, Function)},
+	 * {@link #getOrThrow()} rethrows the original failure.
+	 */
+	private static final class DeferredSupplier implements Supplier<@Nullable Object> {
+
+		private final FutureTask<@Nullable Object> task;
+
+		private DeferredSupplier(Supplier<@Nullable Object> delegate) {
+			this.task = new FutureTask<>(delegate::get);
+		}
+
+		private void run() {
+			this.task.run();
+		}
+
+		@Override
+		public @Nullable Object get() {
+			try {
+				return this.task.get();
+			}
+			catch (InterruptedException e) {
+				Thread.currentThread().interrupt();
+				throw throwAsUncheckedException(e);
+			}
+			catch (ExecutionException e) {
+				Throwable t = e.getCause();
+				if (t == null) {
+					t = e;
+				}
+				UnrecoverableExceptions.rethrowIfUnrecoverable(t);
+				return null;
+			}
+		}
+
+		private @Nullable Object getOrThrow() {
+			try {
+				return this.task.get();
+			}
+			catch (InterruptedException e) {
+				Thread.currentThread().interrupt();
+				throw throwAsUncheckedException(e);
+			}
+			catch (ExecutionException e) {
+				Throwable t = e.getCause();
+				if (t == null) {
+					t = e;
+				}
+				UnrecoverableExceptions.rethrowIfUnrecoverable(t);
+				throw throwAsUncheckedException(t);
+			}
+		}
+	}
+
 	/**
 	 * Thread-safe {@link Supplier} that memoizes the result of calling its
 	 * delegate and ensures it is called at most once.