Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Pankraz76 wants to merge 1 commit into from
+602 −32
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
63 changes: 31 additions & 32 deletions ...ne/src/main/java/org/junit/platform/engine/support/store/NamespacedHierarchicalStore.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ public void close() {
try {
if (this.closeAction != null) {
List<Throwable> failures = new ArrayList<>();
this.storedValues.entrySet().stream() //
storedValues.entrySet().stream() //
.map(e -> e.getValue().evaluateSafely(e.getKey())) //
.filter(it -> it != null && it.value != null) //
.sorted(EvaluatedValue.REVERSE_INSERT_ORDER) //
Expand Down Expand Up @@ -213,11 +213,10 @@ public void close() {
CompositeKey<N> compositeKey = new CompositeKey<>(namespace, key);
StoredValue storedValue = getStoredValue(compositeKey);
if (storedValue == null) {
storedValue = this.storedValues.computeIfAbsent(compositeKey,
__ -> newStoredValue(new MemoizingSupplier(() -> {
rejectIfClosed();
return defaultCreator.apply(key);
})));
storedValue = storedValues.computeIfAbsent(compositeKey, __ -> newValue(new MemoizingSupplier(() -> {
rejectIfClosed();
return defaultCreator.apply(key);
})));
}
return storedValue.evaluate();
}
Expand All @@ -239,24 +238,30 @@ 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");
Copy link
Contributor Author

@Pankraz76 Pankraz76 Dec 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The precondition itself enforces (for good reason, I admit) a specific string pattern, which is beneficial to avoid being overly vague.

Still, the convention principle seems to always win when using the good old POJO approach.

Considering this kind of implementation detail, and in the sake of not making things too DRY, there is some of the same reasoning as correctly mentioned by @mpkorstanje about not introducing too much coupling and extra, extra.

Of course, this is kind of debatable, non-priority, and off-topic—just wanted to give some reasoning.

In: #5209 (comment)

Copy link
Contributor

@mpkorstanje mpkorstanje Dec 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In isolation the reasoning makes sense. But notNull is but one precondition of many. There are also notBlank, containsNoNullElements, ect. So we use Precondition for consistency.

CompositeKey<N> compositeKey = new CompositeKey<>(namespace, key);
StoredValue storedValue = getStoredValue(compositeKey);
requireNonNull(defaultCreator);
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) {
var value = storedValues.compute(compositeKey, (__, currentValue) -> {
if (currentValue == null || currentValue.equals(storedValue)) {
rejectIfClosed();
var computedValue = Preconditions.notNull(defaultCreator.apply(key),
"defaultCreator must not return null");
return newStoredValue(() -> {
return newValue(new MemoizingSupplier(() -> {
rejectIfClosed();
return computedValue;
});
return requireNonNull(defaultCreator.apply(key));
}));
}
return oldStoredValue;
return currentValue;
});
return requireNonNull(newStoredValue.evaluate());
try {
return requireNonNull(value.evaluate());
}
catch (Throwable t) { // remove failed entry to allow retry.
if (value.equals(storedValues.get(compositeKey))) {
storedValues.remove(compositeKey, value);
}
throw t;
}
}
return result;
}
Expand All @@ -281,9 +286,7 @@ public <K, V> Object computeIfAbsent(N namespace, K key, Function<? super K, ? e
public <K, V extends @Nullable Object> @Nullable V getOrComputeIfAbsent(N namespace, K key,
Function<? super K, ? extends V> defaultCreator, Class<V> requiredType)
throws NamespacedHierarchicalStoreException {

Object value = getOrComputeIfAbsent(namespace, key, defaultCreator);
return castToRequiredType(key, value, requiredType);
return castToRequiredType(key, getOrComputeIfAbsent(namespace, key, defaultCreator), requiredType);
}

/**
Expand All @@ -306,9 +309,7 @@ public <K, V> Object computeIfAbsent(N namespace, K key, Function<? super K, ? e
@API(status = MAINTAINED, since = "6.0")
public <K, V> V computeIfAbsent(N namespace, K key, Function<? super K, ? extends V> defaultCreator,
Class<V> requiredType) throws NamespacedHierarchicalStoreException {

Object value = computeIfAbsent(namespace, key, defaultCreator);
return castNonNullToRequiredType(key, value, requiredType);
return castNonNullToRequiredType(key, computeIfAbsent(namespace, key, defaultCreator), requiredType);
}

/**
Expand All @@ -328,8 +329,8 @@ public <K, V> V computeIfAbsent(N namespace, K key, Function<? super K, ? extend
public @Nullable Object put(N namespace, Object key, @Nullable Object value)
throws NamespacedHierarchicalStoreException {
rejectIfClosed();
StoredValue oldValue = this.storedValues.put(new CompositeKey<>(namespace, key), newStoredValue(() -> value));
return StoredValue.evaluateIfNotNull(oldValue);
return StoredValue.evaluateIfNotNull(
storedValues.put(new CompositeKey<>(namespace, key), newValue(() -> value)));
}

/**
Expand All @@ -347,8 +348,7 @@ public <K, V> V computeIfAbsent(N namespace, K key, Function<? super K, ? extend
*/
public @Nullable Object remove(N namespace, Object key) {
rejectIfClosed();
StoredValue previous = this.storedValues.remove(new CompositeKey<>(namespace, key));
return StoredValue.evaluateIfNotNull(previous);
return StoredValue.evaluateIfNotNull(storedValues.remove(new CompositeKey<>(namespace, key)));
}

/**
Expand All @@ -368,16 +368,15 @@ public <K, V> V computeIfAbsent(N namespace, K key, Function<? super K, ? extend
public <T> @Nullable T remove(N namespace, Object key, Class<T> requiredType)
throws NamespacedHierarchicalStoreException {
rejectIfClosed();
Object value = remove(namespace, key);
return castToRequiredType(key, value, requiredType);
return castToRequiredType(key, remove(namespace, key), requiredType);
}

private StoredValue newStoredValue(Supplier<@Nullable Object> value) {
private StoredValue newValue(Supplier<@Nullable Object> value) {
return new StoredValue(this.insertOrderSequence.getAndIncrement(), value);
}

private @Nullable StoredValue getStoredValue(CompositeKey<N> compositeKey) {
StoredValue storedValue = this.storedValues.get(compositeKey);
StoredValue storedValue = storedValues.get(compositeKey);
if (storedValue != null) {
return storedValue;
}
Expand Down
Loading