On Thu, 15 Jun 2023 20:34:53 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).
test/jdk/java/lang/management/ThreadMXBean/Locks.java line 72:
> 70: .filter(Objects::nonNull)
> 71: .filter(i ->
> name.equals(i.getLockOwnerName()))
> 72: .filter(i ->
> !i.getLockName().contains("java.lang.VirtualThread"))
Suggestion:
.filter(i ->
!i.getLockName().contains("java.lang.VirtualThread"))
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/14501#discussion_r1236928027
