On Mon, 16 Mar 2026 20:13:30 GMT, Zhengyu Gu <[email protected]> wrote:
>> Please review this change that fixes crashes by jmm_GetThreadInfo() call.
>>
>> There are several issues:
>> - ThreadIdTable::lazy_initialize() has typical double-checked locking
>> pattern without proper memory barrier, that may result in
>> uninitialized/partial initialized table to be observed by other threads.
>> Added release/acquire barrier to address this issue.
>> - Query ThreadIdTable can race add/remove thread operations. In short, the
>> thread returned from the query may be freed. Fortunately,
>> jmm_GetThreadInfo() acquires stable thread list before query, so we only
>> need to make sure that returned thread is in the list (checking
>> thread->is_exiting() does not help due to the race)
>> - I moved thread Id insertion code from ThreadSMR to Threads, to be
>> symmetric to thread Id removal code.
>>
>> Tests:
>> - [x] Tier1 on Linux and MacOSX (fastdebug)
>> (`tools/javac/annotations/typeAnnotations/IncorrectCastOffsetTest.java`
>> failure seems unrelated, it also fails in master)
>
> Zhengyu Gu has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Update ThreadInfoTest.java from @kevinjwalls
The test is still causing me some issues. IIUC we just continually sample a set
of changing threads so that there are potentially races with terminating
threads, which previously caused the crash - right?
test/hotspot/jtreg/serviceability/threads/ThreadInfoTest.java line 89:
> 87:
> 88: // Count ThreadInfo from array, return how many are non-null.
> 89: private static int countInfo(long[] ids, ThreadInfo[] info) {
`ids` is unused in this function.
test/hotspot/jtreg/serviceability/threads/ThreadInfoTest.java line 107:
> 105: if (info != null) {
> 106: int i = 0;
> 107: for (ThreadInfo ti: info) {
Nit: if you have to manually track the loop index then there is no real point
using a for-each loop - just use a normal for-loop.
test/hotspot/jtreg/serviceability/threads/ThreadInfoTest.java line 141:
> 139: while (true) {
> 140: if (replacing) {
> 141: int replaced = replaceThreads(ids, infos);
The return value is unused and serves no purpose.
test/hotspot/jtreg/serviceability/threads/ThreadInfoTest.java line 169:
> 167: Exception myException = new Exception("Test exception");
> 168: long sleep = Math.max(1, endTimeMs -
> System.currentTimeMillis());
> 169: goSleep(sleep);
Is this really all needed? Any reason not to unlock the lock?
-------------
Changes requested by dholmes (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/30105#pullrequestreview-3965533725
PR Review Comment: https://git.openjdk.org/jdk/pull/30105#discussion_r2951345092
PR Review Comment: https://git.openjdk.org/jdk/pull/30105#discussion_r2951351872
PR Review Comment: https://git.openjdk.org/jdk/pull/30105#discussion_r2951359284
PR Review Comment: https://git.openjdk.org/jdk/pull/30105#discussion_r2951384115
