On Thu, 23 Nov 2023 01:29: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. > > test/hotspot/jtreg/runtime/Monitor/IterateMonitorWithDeadObjectTest.java line > 29: > >> 27: * @summary This locks a monitor, GCs the object, and iterate and perform >> 28: * various iteration and operations over this monitor. >> 29: * @requires os.family == "linux" > > I know the test this was copied from had this but I'm not sure it is actually > a necessary restriction - any Posix platform should work. Though maybe perror > is linux only ... I couldn't find any test filtering for posix, but some tests have: @requires os.family != "windows" That seems to work on my Mac. > test/hotspot/jtreg/runtime/Monitor/IterateMonitorWithDeadObjectTest.java line > 31: > >> 29: * @requires os.family == "linux" >> 30: * @library /testlibrary /test/lib >> 31: * @build IterateMonitorWithDeadObjectTest > > You don't need an explicit `@build` step Sure. This was just copied brought over from your original reproducer. > test/hotspot/jtreg/runtime/Monitor/IterateMonitorWithDeadObjectTest.java line > 66: > >> 64: // dead object. The thread dumping code didn't tolerate such a >> monitor, >> 65: // so run a thread dump and make sure that it doesn't >> crash/assert. >> 66: dumpThreadsWithLockedMonitors(); > > But you've already detached the thread so there is no locked monitor any > longer. And `runTestAndDetachThread()` also did a thread dump. Right. This is left-overs from my earlier attempt. I'll remove this. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/16783#discussion_r1403077397 PR Review Comment: https://git.openjdk.org/jdk/pull/16783#discussion_r1403077988 PR Review Comment: https://git.openjdk.org/jdk/pull/16783#discussion_r1403078851
