On Fri, 24 Nov 2023 09:18:46 GMT, Stefan Karlsson <stef...@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. > > Stefan Karlsson has updated the pull request incrementally with one > additional commit since the last revision: > > Split test and use othervm Sorry I still have quite a few issues with the tests. The new test in particular seems quite difficult to follow. IIUC you basically want to test the "dead object" thread dump scenario in two cases: before the native thread has detached and unlocked the object; after the native thread has detached and unlocked the object. To do the former you need to dump from the native thread; for the latter it could be done in the native thread after the detach, or the main thread after the join. test/hotspot/jtreg/runtime/Monitor/MonitorWithDeadObjectTest.java line 26: > 24: > 25: /* > 26: * @summary This test checks that ObjectMonitors with dead objects don't Please add `@bug` line test/hotspot/jtreg/runtime/Monitor/MonitorWithDeadObjectTest.java line 75: > 73: private static void testDetachThread() { > 74: // Create an ObjectMonitor with a dead object from an > 75: // attached thread. Unclear what the "Detach" in the method name has to do with anything. ?? test/hotspot/jtreg/runtime/Monitor/MonitorWithDeadObjectTest.java line 86: > 84: } > 85: > 86: private static void testDumpThreadsAfterDetachBeforeJoin() { The `AfterDetach` in the name is not accurate. If you don't join the new native thread then you are racing with its execution and you don't know when it will detach. test/hotspot/jtreg/runtime/Monitor/MonitorWithDeadObjectTest.java line 95: > 93: dumpThreadsWithLockedMonitors(); > 94: > 95: joinTestThread(); I'm not seeing the relevance of the join/no-join aspect of these tests. If you join the target thread then you know it has created the "bad" monitor, detached (and so unlocked) and exited; otherwise you are racing with it. test/hotspot/jtreg/runtime/Monitor/MonitorWithDeadObjectTest.java line 100: > 98: private static void testDumpThreadsAfterDetachAfterJoin() { > 99: createMonitorWithDeadObjectNoJoin(); > 100: joinTestThread(); How is this different to just calling `createMonitorWithDeadObject` ? test/hotspot/jtreg/runtime/Monitor/libMonitorWithDeadObjectTest.c line 47: > 45: > 46: #define check(env, what, msg) \ > 47: check_exception((env), (msg)); \ I'm not understanding why you have `check` and `check_exception` here nor why you choose to use one versus the other. ?? test/hotspot/jtreg/runtime/Monitor/libMonitorWithDeadObjectTest.c line 130: > 128: // test provokes that situation and that asserts. > 129: if ((*jvm)->DetachCurrentThread(jvm) != JNI_OK) > die("DetachCurrentThread"); > 130: pthread_exit(NULL); You don't need to call `pthread_exit` - the thread's entry function can simply return. test/hotspot/jtreg/runtime/Monitor/libMonitorWithDeadObjectTest.c line 161: > 159: > 160: if (pthread_attr_init(&attr) != 0) die("pthread_attr_init"); > 161: if (pthread_create(&attacher, &attr, > create_monitor_with_dead_object_in_thread, NULL) != 0) die("pthread_create"); You are not actually using the attr object to change anything. On AIX you may need to explicitly set the stack size. test/hotspot/jtreg/serviceability/jvmti/GetOwnedMonitorInfo/GetOwnedMonitorInfoTest.java line 1: > 1: /* Please update the `@bug` line and update the summary. test/hotspot/jtreg/serviceability/jvmti/GetOwnedMonitorInfo/GetOwnedMonitorInfoTest.java line 53: > 51: private static native boolean hasEventPosted(); > 52: > 53: private static void jniMonitorEnterAndLetObjectDie() { I can see it is convenient to just inject this test case in an existing test, but I'm not sure it is necessarily the right thing to do. Serviceability folk may have a stronger opinion. test/hotspot/jtreg/serviceability/jvmti/GetOwnedMonitorInfo/GetOwnedMonitorInfoTest.java line 58: > 56: // Inject this situation into this test that performs other > 57: // GetOwnedMonitorInfo testing. > 58: Object obj = new Object() { public String toString() {return "";} > }; Nit: the `toString` definition is not needed. This could just be `new Object();`, or `new Object() {};` if you want to introduce a nested class. test/hotspot/jtreg/serviceability/jvmti/GetOwnedMonitorInfo/GetOwnedMonitorInfoTest.java line 78: > 76: final GetOwnedMonitorInfoTest lock = new > GetOwnedMonitorInfoTest(); > 77: > 78: Thread t1 = threadFactory.newThread(() -> { Pre-existing nit: by default virtual threads have no name, so the output in the virtual thread case looks a little odd. Can you add: Thread.currentThread().setName("Worker-Thread"); please. test/hotspot/jtreg/serviceability/jvmti/GetOwnedMonitorInfo/libGetOwnedMonitorInfoTest.c line 270: > 268: Java_GetOwnedMonitorInfoTest_jniMonitorEnter(JNIEnv* env, jclass cls, > jobject obj) { > 269: if ((*env)->MonitorEnter(env, obj) != 0) { > 270: fprintf(stderr, "MonitorEnter failed"); Should this be a fatal error? ------------- Changes requested by dholmes (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16783#pullrequestreview-1749493623 PR Review Comment: https://git.openjdk.org/jdk/pull/16783#discussion_r1405540547 PR Review Comment: https://git.openjdk.org/jdk/pull/16783#discussion_r1405541136 PR Review Comment: https://git.openjdk.org/jdk/pull/16783#discussion_r1405547364 PR Review Comment: https://git.openjdk.org/jdk/pull/16783#discussion_r1405548116 PR Review Comment: https://git.openjdk.org/jdk/pull/16783#discussion_r1405548359 PR Review Comment: https://git.openjdk.org/jdk/pull/16783#discussion_r1405543595 PR Review Comment: https://git.openjdk.org/jdk/pull/16783#discussion_r1405551073 PR Review Comment: https://git.openjdk.org/jdk/pull/16783#discussion_r1405544843 PR Review Comment: https://git.openjdk.org/jdk/pull/16783#discussion_r1405548997 PR Review Comment: https://git.openjdk.org/jdk/pull/16783#discussion_r1405550273 PR Review Comment: https://git.openjdk.org/jdk/pull/16783#discussion_r1405549538 PR Review Comment: https://git.openjdk.org/jdk/pull/16783#discussion_r1405552370 PR Review Comment: https://git.openjdk.org/jdk/pull/16783#discussion_r1405551461
