RFR: 8306446: java/lang/management/ThreadMXBean/Locks.java transient failures [v3]

[Chris Plummer]

On Wed, 21 Jun 2023 16:56:08 GMT, Kevin Walls <kevinw at openjdk.org> wrote:

>> This test iterates an array of ThreadInfos in a few places (e.g. in the method doCheck()), and needs to tolerate and ignore nulls, in case a thread finishes and the test hits an NPE.
>> 
>> There are other calls like "TM.getThreadInfo(tid).getLockName()" which might often be risky, but if the threads are blocked as they are here, they can't be terminating, so this usage is safe.
>> 
>> 
>> The test has additional problems when started in a virtual thread.  ThreadMXBean.getThreadInfo() methods only return a ThreadInfo for platform threads.  The test needs to avoid some checks if mainThread is virtual.
>> 
>> In assertNoLock, it needs to not object to a thread holding a lock on a VirtualThread object is not relevant.
>> Also the loop in doChecks which follows a chain of locks... This needs to recognise that ForkJoinPool thead is not worth pursuing.  It's not one of the very narrow set of threads this test cares about.
>> 
>> Despite these exclusions, the test does some reasonable verification work when MainThread is virtual.  This test historically cam in with a general "JVM monitoring and management API" change, it is not testing a particular fix.
>> 
>> 
>> There's a failure condition in doCheck() which will not make the test fail: if it logs "TEST FAILED" in its final for loop, there is no failure.  Make the loop count the failures, and throw if there are any.
>> 
>> Also, while looking into this...  The variable names in some methods are confusing. In checkBlockedObject(), let's use "threadName" rather than "result" if we are finding a thread name, and let's not reuse the same result variable for a lockName later in the method.
>> 
>> The logs from this test are hard to read and verify, I find it better if the lock objects OBJB and OBJC are of classes other than Object, so you get to read, e.g.:
>> LockAThread blocked on Locks$ObjectB at 4691fdfd
>> (ObjectB, not just Object).
>
> Kevin Walls has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Comment update

Changes requested by cjplummer (Reviewer).

test/jdk/java/lang/management/ThreadMXBean/Locks.java line 67:

> 65:         }
> 66:         String name = t.getName();
> 67:         // Carrier Thread can hold a lock on a VirtualThread, which we ignore.

This comment's placement can make the user think the whole point of the code that follows is to ignore virtual threads here. Maybe it would be better if placed right before the added filter() call.

test/jdk/java/lang/management/ThreadMXBean/Locks.java line 371:

> 369: 
> 370:                 assertThreadState(t2, Thread.State.BLOCKED);
> 371:                 if (!mainThread.isVirtual()) {

Although the you covered the reason for this exclusion in the PR description, I think a comment is warranted here, and also for line 380 below.

test/jdk/java/lang/management/ThreadMXBean/Locks.java line 450:

> 448:         for (ThreadInfo info : infos) {
> 449:             if (info == null) {
> 450:                 continue; // Missing thread, ignore

Maybe comment on why it might be missing (thread completed already).

test/jdk/java/lang/management/ThreadMXBean/Locks.java line 474:

> 472:                 lock = ownerInfo.getLockName();
> 473:                 continue;
> 474:             }

What happens if you don't exclude these ForkJoinPool threads? What about the myriad of other threads that the JVM always starts up. Should they not also be skipped if you are going to skip ForkJoinPool threads?

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

PR Review: https://git.openjdk.org/jdk/pull/14501#pullrequestreview-1548235473
PR Review Comment: https://git.openjdk.org/jdk/pull/14501#discussion_r1275268210
PR Review Comment: https://git.openjdk.org/jdk/pull/14501#discussion_r1275275010
PR Review Comment: https://git.openjdk.org/jdk/pull/14501#discussion_r1275280004
PR Review Comment: https://git.openjdk.org/jdk/pull/14501#discussion_r1275282729