On Thu, 23 Nov 2023 01:27:24 GMT, David Holmes <dhol...@openjdk.org> wrote:
>> In the rewrites made for: >> [JDK-8318757](https://bugs.openjdk.org/browse/JDK-8318757) `VM_ThreadDump >> asserts in interleaved ObjectMonitor::deflate_monitor calls` >> >> I removed the filtering of *owned ObjectMonitors with dead objects*. The >> reasoning was that you should never have an owned ObjectMonitor with a dead >> object. I added an assert to check this assumption. It turns out that the >> assumption was wrong *if* you use JNI to call MonitorEnter and then remove >> all references to the locked object. >> >> The provided tests provoke this assert form: >> * the JNI thread detach code >> * thread dumping with locked monitors, and >> * the JVMTI GetOwnedMonitorInfo API. >> >> While investigating this we've found that the thread detach code becomes >> more correct when this filter was removed. Previously, the locked monitors >> never got unlocked because the ObjectMonitor iterator never exposed these >> monitors to the JNI detach code that unlocks the thread's monitors. That bug >> caused an ObjectMonitor leak. So, for this case I'm leaving these >> ObjectMonitors unfiltered so that we don't reintroduce the leak. >> >> The thread dumping case doesn't tolerate ObjectMonitor with dead objects, so >> I'm filtering those in the closure that collects ObjectMonitor. Side note: >> We have discussions about ways to completely rewrite this by letting each >> thread have thread-local information about JNI held locks. If we have this >> we could probably throw away the entire ObjectMonitorDump hashtable, and its >> walk of the `_in_use_list.`. >> >> For GetOwnedMonitorInfo it is unclear if we should expose these weird >> ObjectMonitor. If we do, then the users can detect that a thread holds a >> lock with a dead object, and the code will return NULL as one of the "owned >> monitors" returned. I don't think that's a good idea, so I'm filtering out >> these ObjectMonitor for those calls. >> >> Test: the written tests with and without the fix. Tier1-Tier3, so far. > > src/hotspot/share/runtime/vmOperations.cpp line 354: > >> 352: // alive. Filter out monitors with dead objects. >> 353: return; >> 354: } > > I don't think we need to do this, but even without this filtering I ran a > number of tests and was unable to demonstrate any problem. The JNI locked > monitor seems to be "invisible" to the frame that locked it and so the thread > dump never encounters it. Were you able to provoke a failure here or is this > defensive programming? I provoked test failures for all paths I filtered. If I remove this check and run: make -C ../build/fastdebug test TEST=test/hotspot/jtreg/runtime/Monitor/IterateMonitorWithDeadObjectTest.java JTREG="JAVA_OPTIONS=-XX:+UseZGC" I hit this assert: # Internal Error (/home/stefank/git/jdk/open/src/hotspot/share/services/management.cpp:1274), pid=1546709, tid=1546754 # assert(object != nullptr) failed: must be a Java object ... V [libjvm.so+0x1330ce8] jmm_DumpThreads+0x1a48 (management.cpp:1274) j sun.management.ThreadImpl.dumpThreads0([JZZI)[Ljava/lang/management/ThreadInfo;+0 java.management@22-internal j sun.management.ThreadImpl.dumpAllThreads(ZZI)[Ljava/lang/management/ThreadInfo;+28 java.management@22-internal j sun.management.ThreadImpl.dumpAllThreads(ZZ)[Ljava/lang/management/ThreadInfo;+5 java.management@22-internal j IterateMonitorWithDeadObjectTest.dumpThreadsWithLockedMonitors()V+7 j IterateMonitorWithDeadObjectTest.main([Ljava/lang/String;)V+11 If I remove that assert I hit an NPE in the Java layer: java.lang.NullPointerException: Cannot invoke "Object.getClass()" because "lock" is null at java.management/java.lang.management.ThreadInfo.<init>(ThreadInfo.java:172) at java.management/sun.management.ThreadImpl.dumpThreads0(Native Method) at java.management/sun.management.ThreadImpl.dumpAllThreads(ThreadImpl.java:518) at java.management/sun.management.ThreadImpl.dumpAllThreads(ThreadImpl.java:506) at IterateMonitorWithDeadObjectTest.dumpThreadsWithLockedMonitors(IterateMonitorWithDeadObjectTest.java:44) at IterateMonitorWithDeadObjectTest.main(IterateMonitorWithDeadObjectTest.java:66) at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103) at java.base/java.lang.reflect.Method.invoke(Method.java:580) at com.sun.javatest.regtest.agent.MainActionHelper$AgentVMRunnable.run(MainActionHelper.java:333) at java.base/java.lang.Thread.run(Thread.java:1570) ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/16783#discussion_r1403052907
