On Thu, 10 Sep 2026 05:50:29 GMT, Denghui Dong <[email protected]> wrote:
>> Hi, >> >> Please help review this change that fixes crash in >> ThreadSnapshot::initialize. >> >> After [JDK-8323792](https://bugs.openjdk.org/browse/JDK-8323792), we can >> still see the same crash reported in >> [JDK-8374820](https://bugs.openjdk.org/browse/JDK-8374820) and >> [JDK-8346980](https://bugs.openjdk.org/browse/JDK-8346980). >> >> Here is a reproducer: >> >> >> import java.lang.management.ManagementFactory; >> import java.lang.management.ThreadInfo; >> import java.lang.management.ThreadMXBean; >> import java.util.Arrays; >> import java.util.Objects; >> import java.util.concurrent.atomic.AtomicLong; >> import java.util.concurrent.locks.LockSupport; >> >> public class ThreadSnapshotRace { >> >> public static void main(String[] args) throws Exception { >> Thread producer = new Thread(() -> { >> AtomicLong counter = new AtomicLong(); >> long total = 0; >> while (true) { >> long c = counter.incrementAndGet(); >> total++; >> Thread.ofVirtual().name("vthread").start(() -> { >> counter.decrementAndGet(); >> }); >> if (total % 10_000_000 == 0) { >> System.out.println(total); >> } >> if (c >= 20_000_000) { >> do { >> try { >> Thread.sleep(50); >> } catch (Exception e) { >> } >> } while (counter.get() > 0); >> } >> } >> }); >> producer.start(); >> >> Thread.sleep(1000); >> >> Thread consumer = new Thread(() -> { >> ThreadMXBean bean = ManagementFactory.getThreadMXBean(); >> long[] ids = carrierIds(bean); >> while (true) { >> ThreadInfo[] infos = bean.getThreadInfo(ids); >> if (infos.length == 0) { >> System.out.println("?"); >> } >> } >> }); >> consumer.start(); >> } >> >> static long[] carrierIds(ThreadMXBean bean) { >> long[] all = bean.getAllThreadIds(); >> long[] carriers = Arrays.stream(bean.getThreadInfo(all)) >> .filter(Objects::nonNull) >> .filter(ti -> >> ti.getThreadName().startsWith("ForkJoinPool-1-worker")) >> .mapToLong(ThreadInfo::getThreadId) >> .toArray(); >> return carriers; >> } >> } >>... > > Denghui Dong has updated the pull request incrementally with one additional > commit since the last revision: > > add test src/hotspot/share/services/threadService.cpp line 920: > 918: > 919: oop vthread = thread->vthread(); > 920: if (vthread != nullptr && vthread != threadObj) { // ThreadSnapshot > only captures platform threads While stress testing the changes with the extra assert, I found a few failures with `-XX:+CheckUnhandledOops` due to now taking this branch for the platform thread case. Turns out the call to `check_for_dangling_thread_pointer` in `ThreadService::get_current_contended_monitor` above, acquires the `Threads_lock` and that sets `threadObj` to `BAD_OOP_ADDR`, so the oop comparison fails. This causes test `SuspendResume3.java` to timeout for example. The old code didn’t suffer from this because of the `is_vthread_mounted()` check. Simplest fix would be to reload the `threadObj` oop. Or maybe better just use `java_lang_VirtualThread::is_instance(vthread)` instead of comparing oops, which is easier to read (already has null check). test/hotspot/jtreg/serviceability/threads/ThreadSnapshotRaceTest.java line 36: > 34: * @test > 35: * @bug 8392031 > 36: * @summary Make sure that ThreadSnapshot::initialize does not crash JVM Suggestion: * @summary Make sure that ThreadSnapshot::initialize does not crash JVM * @requires vm.continuations test/hotspot/jtreg/serviceability/threads/ThreadSnapshotRaceTest.java line 53: > 51: counter.decrementAndGet(); > 52: }); > 53: if (c >= 20_000_000) { I got intermittent OOM errors with this value, maybe tune it down to 1M? test/hotspot/jtreg/serviceability/threads/ThreadSnapshotRaceTest.java line 72: > 70: while (true) { > 71: ThreadInfo[] infos = bean.getThreadInfo(ids); > 72: assertTrue(infos.length > 0); Note that when calling `carrierIds(bean)` we still can’t guarantee the first virtual thread already started (no FJP workers), so we could hit this assert. test/hotspot/jtreg/serviceability/threads/ThreadSnapshotRaceTest.java line 77: > 75: consumer.start(); > 76: > 77: Thread.sleep(10_000); We should join consumer and producer before exiting. You could add volatile `stop`, set it to true here, and have the workers loop on that. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/32788#discussion_r3984766809 PR Review Comment: https://git.openjdk.org/jdk/pull/32788#discussion_r3984803558 PR Review Comment: https://git.openjdk.org/jdk/pull/32788#discussion_r3984805863 PR Review Comment: https://git.openjdk.org/jdk/pull/32788#discussion_r3984820342 PR Review Comment: https://git.openjdk.org/jdk/pull/32788#discussion_r3984825477
