RFR: 8356658: java/foreign/TestBufferStackStress2.java failed again with junit action timed out [v2]

[Jaikiran Pai]

On Mon, 12 May 2025 09:32:11 GMT, Per Minborg <pminborg at openjdk.org> wrote:

>> This PR proposes to add a safety net around closing the executor. Apparently, in some rare configuration, the VT is not run/notified correctly.
>> 
>> Not completing the test for such a configuration is unlikely to mask potential issues that this test is supposed to reveal.
>
> Per Minborg has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Add shutdown

test/jdk/java/foreign/TestBufferStackStress2.java line 139:

> 137:             // This is a corner case for rare configurations
> 138:             System.out.println(duration(begin) + "ABORTED");
> 139:             System.exit(0);

Hello Per, a `System.exit(...)` call isn't recommended in a jtreg test (https://openjdk.org/jtreg/faq.html#should-a-test-call-the-system.exit-method)

On a general note though, I think this test might need a different solution to address the issue. For one, I think it might be better to use `@run junit/othervm` instead of the current `@run junit` to prevent any agent VM specific state to interfere with this test.

Secondly, I think in its current form the test may still have a race. The main thread which does the `quiescent.notify();` may execute before the other thread reaches the `quiescent.wait();`. That can cause the other thread to wait forever. I think to avoid this, a `CountDownLatch` might be more appropriate. Something like (the untested change below):


diff --git a/test/jdk/java/foreign/TestBufferStackStress2.java b/test/jdk/java/foreign/TestBufferStackStress2.java
index 8ec5d512ff6..0ba28ad6f9e 100644
--- a/test/jdk/java/foreign/TestBufferStackStress2.java
+++ b/test/jdk/java/foreign/TestBufferStackStress2.java
@@ -38,6 +38,7 @@
 import java.lang.foreign.ValueLayout;
 import java.time.Duration;
 import java.time.temporal.ChronoUnit;
+import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.Executors;
 import java.util.concurrent.ForkJoinWorkerThread;
 import java.util.concurrent.atomic.AtomicBoolean;
@@ -78,7 +79,7 @@ void movingVirtualThreadWithGc() throws InterruptedException {
 
         var done = new AtomicBoolean();
         var completed = new AtomicBoolean();
-        var quiescent = new Object();
+        var quiescentLatch = new CountDownLatch(1);
         var executor = Executors.newVirtualThreadPerTaskExecutor();
 
         executor.submit(() -> {
@@ -93,12 +94,11 @@ void movingVirtualThreadWithGc() throws InterruptedException {
             try (Arena arena = pool.pushFrame(SMALL_ALLOC_SIZE, 1)) {
                 MemorySegment segment = arena.allocate(SMALL_ALLOC_SIZE);
                 done.set(true);
-                synchronized (quiescent) {
-                    try {
-                        quiescent.wait();
-                    } catch (Throwable ex) {
-                        throw new AssertionError(ex);
-                    }
+                // wait for ForkJoinPool to contract
+                try {
+                    quiescentLatch.await();
+                } catch (Throwable ex) {
+                    throw new AssertionError(ex);
                 }
                 System.out.println(duration(begin) + "ACCESSING SEGMENT");
 
@@ -123,10 +123,7 @@ void movingVirtualThreadWithGc() throws InterruptedException {
         } while (count > 0);
 
         System.out.println(duration(begin) + "FJP HAS CONTRACTED");
-
-        synchronized (quiescent) {
-            quiescent.notify();
-        }
+        quiescentLatch.countDown(); // notify the thread that accesses the MemorySegment
 
         System.out.println(duration(begin) + "CLOSING EXECUTOR");
         executor.close();

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/25177#discussion_r2084250912