On Wed, 26 Jul 2023 20:14:16 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: > > feedback This looks pretty good to me. Is the test not failing anymore with your update? ------------- PR Review: https://git.openjdk.org/jdk/pull/14501#pullrequestreview-1550261930
