[tryout fix] concurrency bug in NamespacedHierarchicalStore#computeIfAbsent(Object, Object, Function) #5171 #5209

Signed-off-by: Vincent Potucek <[email protected]>

Author: Vincent Potucek

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

@@ -17,6 +17,13 @@ repository on GitHub.
 ==== Bug Fixes
 
 * Make `ConsoleLauncher` compatible with JDK 26 by avoiding final field mutations.
+* Fix a concurrency bug in `NamespacedHierarchicalStore#computeIfAbsent(Object, Object, Function)` where
+  the `defaultCreator` function was executed while holding the store's internal
+  map lock. Under parallel execution, this could cause threads using the store to
+  block each other and temporarily see a missing or incorrectly initialized state
+  for values created via `computeIfAbsent`. The method now evaluates
+  `defaultCreator` outside the critical section using a memorizing supplier,
+  aligning its behavior with the deprecated `getOrComputeIfAbsent`.
 
 [[v6.0.2-junit-platform-deprecations-and-breaking-changes]]
 ==== Deprecations and Breaking Changes

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

@@ -16,6 +16,7 @@
 import static org.apiguardian.api.API.Status.EXPERIMENTAL;
 import static org.apiguardian.api.API.Status.MAINTAINED;
 import static org.junit.platform.commons.util.ExceptionUtils.throwAsUncheckedException;
+import static org.junit.platform.commons.util.Preconditions.notNull;
 import static org.junit.platform.commons.util.ReflectionUtils.getWrapperType;
 import static org.junit.platform.commons.util.ReflectionUtils.isAssignableTo;
 
@@ -31,7 +32,6 @@
 
 import org.apiguardian.api.API;
 import org.jspecify.annotations.Nullable;
-import org.junit.platform.commons.util.Preconditions;
 import org.junit.platform.commons.util.UnrecoverableExceptions;
 
 /**
@@ -209,7 +209,7 @@ public void close() {
 	@API(status = DEPRECATED, since = "6.0")
 	public <K, V extends @Nullable Object> @Nullable Object getOrComputeIfAbsent(N namespace, K key,
 			Function<? super K, ? extends V> defaultCreator) {
-		Preconditions.notNull(defaultCreator, "defaultCreator must not be null");
+		notNull(defaultCreator, "defaultCreator must not be null");
 		CompositeKey<N> compositeKey = new CompositeKey<>(namespace, key);
 		StoredValue storedValue = getStoredValue(compositeKey);
 		if (storedValue == null) {
@@ -239,28 +239,36 @@ public void close() {
 	 */
 	@API(status = MAINTAINED, since = "6.0")
 	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);
+		notNull(defaultCreator, "defaultCreator must not be null");
+		var compositeKey = new CompositeKey<>(namespace, key);
+		var 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;
-					});
+			var value = storedValues.compute(compositeKey,
+				(__, currentValue) -> currentValue == null || currentValue.equals(storedValue)
+						? storeNewValue(key, defaultCreator)
+						: currentValue);
+			try {
+				return requireNonNull(value.evaluate());
+			}
+			catch (Throwable t) { // remove failed entry to allow retry.
+				if (value.equals(storedValues.get(compositeKey))) {
+					storedValues.remove(compositeKey, value);
 				}
-				return oldStoredValue;
-			});
-			return requireNonNull(newStoredValue.evaluate());
+				throw t;
+			}
 		}
 		return result;
 	}
 
+	private <K, V> StoredValue storeNewValue(K key, Function<? super K, ? extends V> defaultCreator) {
+		rejectIfClosed();
+		return newStoredValue(new MemoizingSupplier(() -> {
+			rejectIfClosed();
+			return requireNonNull(defaultCreator.apply(key));
+		}));
+	}
+
 	/**
 	 * Get the value stored for the supplied namespace and key in this store or
 	 * the parent store, if present, or call the supplied function to compute it
@@ -388,7 +396,7 @@ private StoredValue newStoredValue(Supplier<@Nullable Object> value) {
 	}
 
 	private <T> @Nullable T castToRequiredType(Object key, @Nullable Object value, Class<T> requiredType) {
-		Preconditions.notNull(requiredType, "requiredType must not be null");
+		notNull(requiredType, "requiredType must not be null");
 		if (value == null) {
 			return null;
 		}
@@ -419,8 +427,8 @@ private void rejectIfClosed() {
 	private record CompositeKey<N>(N namespace, Object key) {
 
 		CompositeKey {
-			Preconditions.notNull(namespace, "namespace must not be null");
-			Preconditions.notNull(key, "key must not be null");
+			notNull(namespace, "namespace must not be null");
+			notNull(key, "key must not be null");
 		}
 
 	}