RFR: 8345266: java/util/concurrent/locks/StampedLock/OOMEInStampedLock.java JTREG_TEST_THREAD_FACTORY=Virtual fails with OOME

[Alan Bateman]

On Tue, 17 Dec 2024 00:44:42 GMT, Patricio Chilano Mateo <pchilanomate at openjdk.org> wrote:

> Please review this small fix. The call to `Continuation::try_preempt()` can fail and return with a pending OOM exception due to insufficient memory while trying to allocate a new stackChunk. On this particular `OOMEInStampedLock.java` test, the OOM is thrown when returning from Object.wait, which is unexpected and causes the test to fail. But this could also happen when a virtual thread returns from monitorenter, which would cause an assert when coming from compiled code (see `SharedRuntime::monitor_enter_helper()`). Since a failing `try_preempt()` call will just prevent the vthread from being unmounted and force it to follow the normal platform thread code there is no need to throw OOM. The fix is to just clear the pending exception if set, and let future bytecodes handle the OOM condition if still present.
> 
> I verified that test `OOMEInStampedLock.java` now passes with the fix. I also added a new more specific test which fails without this fix and passes with it. I also run the change through mach5 tiers1-3.
> 
> Thanks,
> Patricio

The update to Continuation::try_preempt looks good to me. Initially I thought it might be problematic to clear the exception in the face of an async exception from JVMTI StopThread but that is limited to the suspended case so can't happen.

Tests for OOME scenarios have a history of being troublesome. This one looks manageable but I worry a bit that it might need further work in the future to keep stable.

test/jdk/java/lang/Thread/virtual/MonitorOOM.java line 35:

> 33: /*
> 34:  * @test id=wait
> 35:  * @bug 8345266

We don't have to list the @bug tag in all test descriptions if you don't want. This comes up when adding more test cases to an existing test where we add to the @bug tag.

test/jdk/java/lang/Thread/virtual/MonitorOOM.java line 45:

> 43: import java.util.concurrent.atomic.AtomicReference;
> 44: 
> 45: public class MonitorOOM {

For naming, something like MonitorEnterWaitOOME might be clearer.

test/jdk/java/lang/Thread/virtual/MonitorOOM.java line 65:

> 63:                     synchronized (lock) {
> 64:                         if (testWait) {
> 65:                             lock.wait(5);

This is a timed wait, I suppose we should also test the untimed wait.

test/jdk/java/lang/Thread/virtual/MonitorOOM.java line 79:

> 77:             canFillHeap.set(true);
> 78:             awaitTrue(heapFilled);
> 79:             loopMillis(100); // allow vthread to reach synchronized

100ms may not be enough, I wonder if this could poll the thread state until BLOCKED.

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

PR Review: https://git.openjdk.org/jdk/pull/22780#pullrequestreview-2508647543
PR Review Comment: https://git.openjdk.org/jdk/pull/22780#discussion_r1888379166
PR Review Comment: https://git.openjdk.org/jdk/pull/22780#discussion_r1888379988
PR Review Comment: https://git.openjdk.org/jdk/pull/22780#discussion_r1888380523
PR Review Comment: https://git.openjdk.org/jdk/pull/22780#discussion_r1888386290