On Wed, 21 Jun 2023 16:56:08 GMT, Kevin Walls <kev...@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@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
