Concurrency problem in `NamespacedHierarchicalStore#computeIfAbsent`

[danaebu]

Steps to reproduce

1. Clone https://github.com/assertj/assertj locally
2. Setup project according to CONTRIBUTING.md
3. In line: assertj/assertj@f90a09a#diff-5d92da3c93616cd468991f168636c72526bc929869ee121efa0a1478600c950cR45 remove the @Disabled annotation
4. Execute ./mvnw clean verify

Expected

Tests run successfully, all tests pass.

Actual

When running all tests, the test in SoftAssertionsExtension_PER_CLASS_Concurrency_Test.java fails sometimes, and on some machines. When running it individually using the maven wrapper and through the IDE, it passes consistently, which is why we believe it to be a concurrency issue.

The failure has the following output:

[ERROR] Failures: 
[ERROR]   SoftAssertionsExtension_PER_CLASS_Concurrency_Test.concurrent_tests_with_explicit_per_class_annotation_do_not_interfere:116 Test Event Statistics (2 failures)
        org.opentest4j.AssertionFailedError: started ==> expected: <2> but was: <0>                                                                                                                      
        org.opentest4j.AssertionFailedError: failed ==> expected: <2> but was: <0>            

We identified that the change that introduced the flakiness is this line: assertj/assertj@bcb4e14#diff-1fd14641c6731a83101088806368089b7af173b3a7424a504654008cab1a0dddR411

Here it was changed from using the deprecated JUnit method getOrComputeIfAbsent to computeIfAbsent (according to the JUnit Javadoc). Reverting the change in this line only already resolves the flakiness.

The test in SoftAssertionsExtension_PER_CLASS_Concurrency_Test.java executes soft assertions concurrently, thus also executing getOrComputeIfAbsent/computeIfAbsent concurrently, which suggests that there is a concurrency issue in computeIfAbsent that wasn't in getOrComputeWithAbsent.

Analysis

We noticed one of the substantial changes from the getOrComputeIfAbsent implementation to the computeIfAbsent implementation is that the call on the ConcurrentMap was changed from computeIfAbsent to compute, see:

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

Line 247 in 19557e4

StoredValue newStoredValue = this.storedValues.compute(compositeKey, (__, oldStoredValue) -> {

Our hypothesis is, that if two threads are executing NamespacedHierarchicalStore#computeIfAbsent in parallel, then if both threads are executing at the same speed where both end up executing this.storedValues.compute(... at the same time, the first thread will execute properly, while the second thread will overwrite the value in the Map. Previously in getOrComputeIfAbsent, since this.storedValues.computeIfAbsent was used, it would guarantee that the second thread would not overwrite the value, as the ConcurrentMap is enforcing synchronization properly.

We were able to verify that temporarily changing the offending line in NamespacedHierarchicalStore#computeIfAbsent to use computeIfAbsent instead of compute, and then publishing it to Maven local, using that build in AssertJ reliably resolves the flakiness even when running the tests in parallel and using getStore(context).computeIfAbsent in assertj/assertj@bcb4e14#diff-1fd14641c6731a83101088806368089b7af173b3a7424a504654008cab1a0dddR411
You can find this quick and dirty change we made where the issue no longer occurred here: danaebu@9384b94

See related issue on the flakiness in AssertJ here: assertj/assertj#1996

Context

• Used versions (Jupiter/Vintage/Platform): 6.0.1
• Build Tool/IDE: Maven/IntelliJ (but can be reproduced outside the IDE by using the maven commands on the terminal as well)

As a ConcurrentMap is used, we assume the implementation NamespacedHierarchicalStore#computeIfAbsent is supposed to be thread-safe.

Was found together with @martinfrancois at Hack Commit Push

Deliverables

• NamespacedHierarchicalStore#computeIfAbsent is thread-safe
• Unit test is added to JUnit as well that reproduces this issue reliably to avoid regressions.

[scordio]

Clone https://github.com/assertj/assertj locally

As I reverted the upgrade to JUnit 6, please use assertj/assertj@f90a09a to verify the issue.