On Mon, 12 May 2025 09:32:11 GMT, Per Minborg <pminb...@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
