Skip to content

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

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
+918 −17
Closed

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

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
7 changes: 7 additions & 0 deletions documentation/modules/ROOT/partials/release-notes/release-notes-6.0.2.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
42 changes: 25 additions & 17 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 @@ -214,7 +214,7 @@ public void close() {
StoredValue storedValue = getStoredValue(compositeKey);
if (storedValue == null) {
storedValue = this.storedValues.computeIfAbsent(compositeKey,
__ -> newStoredValue(new MemoizingSupplier(() -> {
__ -> newStoredValue(new MemorizingSupplier(() -> {
rejectIfClosed();
return defaultCreator.apply(key);
})));
Expand All @@ -240,27 +240,35 @@ 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);
Copy link
Contributor Author

@Pankraz76 Pankraz76 Dec 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I see that it is harder to review. On the other hand, it is also difficult (for me) to access this code without. This can be helpful when working on a close, low-level perspective.

I do not consider code at all, because everything ultimately becomes some kind of random implementation detail.

If I need to consider code seriously, I must structure it properly and understand how it actually flows, while adhering to best practices and SOLID design principles that reflect what the code is truly doing and how it whispers how it wants to be structured. I know not everyone has the sense to recognize this level of intent.

I cannot process an endless number of requirements and concerns. I can only operate like a computer: one thing at a time, within a very limited scope—effectively binary. While 0 and 1 represent two states, they never truly add up to 2. Because of this, I have to work at a one-to-one level; anything beyond that exceeds my strict mental limitations. Please excuse my shortcomings in this regard.

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 MemorizingSupplier(() -> {
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
Expand Down Expand Up @@ -469,7 +477,7 @@ private void close(CloseAction<N> closeAction) throws Throwable {
*
* @see StoredValue
*/
private static class MemoizingSupplier implements Supplier<@Nullable Object> {
private static class MemorizingSupplier implements Supplier<@Nullable Object> {

private static final Object NO_VALUE_SET = new Object();

Expand All @@ -478,7 +486,7 @@ private static class MemoizingSupplier implements Supplier<@Nullable Object> {
@Nullable
private volatile Object value = NO_VALUE_SET;

private MemoizingSupplier(Supplier<@Nullable Object> delegate) {
Copy link
Contributor Author

@Pankraz76 Pankraz76 Dec 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

seems like a typo missing r

Copy link
Contributor

@mpkorstanje mpkorstanje Dec 13, 2025

Choose a reason for hiding this comment

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

They're similar words but mean different things.

https://en.wikipedia.org/wiki/Memoization

private MemorizingSupplier(Supplier<@Nullable Object> delegate) {
this.delegate = delegate;
}

Expand Down
Loading