From 274c805c5137d9080a7670d864ecca8a0befc3f6 Mon Sep 17 00:00:00 2001 From: David Holmes Date: Tue, 16 Apr 2024 00:48:15 +0000 Subject: [PATCH] 8327743: JVM crash in hotspot/share/runtime/javaThread.cpp - failed: held monitor count should be equal to jni: 0 != 1 Co-authored-by: Fredrik Bredberg Reviewed-by: rehn, fbredberg, pchilanomate, rrich --- .../cpu/aarch64/sharedRuntime_aarch64.cpp | 42 ++- src/hotspot/cpu/ppc/sharedRuntime_ppc.cpp | 55 +++- src/hotspot/cpu/riscv/sharedRuntime_riscv.cpp | 43 ++- src/hotspot/cpu/x86/sharedRuntime_x86_64.cpp | 42 ++- src/hotspot/share/runtime/javaThread.cpp | 47 ++- src/hotspot/share/runtime/javaThread.hpp | 1 + src/hotspot/share/runtime/sharedRuntime.cpp | 14 + src/hotspot/share/runtime/sharedRuntime.hpp | 3 + .../vthread/JNIMonitor/JNIMonitor.java | 277 ++++++++++++++++-- .../GetOwnedMonitorInfoTest.java | 11 +- 10 files changed, 491 insertions(+), 44 deletions(-) diff --git a/src/hotspot/cpu/aarch64/sharedRuntime_aarch64.cpp b/src/hotspot/cpu/aarch64/sharedRuntime_aarch64.cpp index b200fb4c4b0..a2600825622 100644 --- a/src/hotspot/cpu/aarch64/sharedRuntime_aarch64.cpp +++ b/src/hotspot/cpu/aarch64/sharedRuntime_aarch64.cpp @@ -1038,9 +1038,49 @@ static void continuation_enter_cleanup(MacroAssembler* masm) { __ stop("incorrect sp1"); __ bind(OK); #endif - __ ldr(rscratch1, Address(sp, ContinuationEntry::parent_cont_fastpath_offset())); __ str(rscratch1, Address(rthread, JavaThread::cont_fastpath_offset())); + + if (CheckJNICalls) { + // Check if this is a virtual thread continuation + Label L_skip_vthread_code; + __ ldrw(rscratch1, Address(sp, ContinuationEntry::flags_offset())); + __ cbzw(rscratch1, L_skip_vthread_code); + + // If the held monitor count is > 0 and this vthread is terminating then + // it failed to release a JNI monitor. So we issue the same log message + // that JavaThread::exit does. + __ ldr(rscratch1, Address(rthread, JavaThread::jni_monitor_count_offset())); + __ cbz(rscratch1, L_skip_vthread_code); + + // Save return value potentially containing the exception oop in callee-saved R19. + __ mov(r19, r0); + __ call_VM_leaf(CAST_FROM_FN_PTR(address, SharedRuntime::log_jni_monitor_still_held)); + // Restore potential return value. + __ mov(r0, r19); + + // For vthreads we have to explicitly zero the JNI monitor count of the carrier + // on termination. The held count is implicitly zeroed below when we restore from + // the parent held count (which has to be zero). + __ str(zr, Address(rthread, JavaThread::jni_monitor_count_offset())); + + __ bind(L_skip_vthread_code); + } +#ifdef ASSERT + else { + // Check if this is a virtual thread continuation + Label L_skip_vthread_code; + __ ldrw(rscratch1, Address(sp, ContinuationEntry::flags_offset())); + __ cbzw(rscratch1, L_skip_vthread_code); + + // See comment just above. If not checking JNI calls the JNI count is only + // needed for assertion checking. + __ str(zr, Address(rthread, JavaThread::jni_monitor_count_offset())); + + __ bind(L_skip_vthread_code); + } +#endif + __ ldr(rscratch1, Address(sp, ContinuationEntry::parent_held_monitor_count_offset())); __ str(rscratch1, Address(rthread, JavaThread::held_monitor_count_offset())); diff --git a/src/hotspot/cpu/ppc/sharedRuntime_ppc.cpp b/src/hotspot/cpu/ppc/sharedRuntime_ppc.cpp index 666b46817f9..12afe49537d 100644 --- a/src/hotspot/cpu/ppc/sharedRuntime_ppc.cpp +++ b/src/hotspot/cpu/ppc/sharedRuntime_ppc.cpp @@ -1,6 +1,6 @@ /* - * Copyright (c) 1997, 2023, Oracle and/or its affiliates. All rights reserved. - * Copyright (c) 2012, 2023 SAP SE. All rights reserved. + * Copyright (c) 1997, 2024, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2012, 2024 SAP SE. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -1637,7 +1637,7 @@ static void fill_continuation_entry(MacroAssembler* masm, Register reg_cont_obj, // None. // // Kills: -// R8_ARG6, R9_ARG7, R10_ARG8 +// R8_ARG6, R9_ARG7, R10_ARG8, R15_esp // static void continuation_enter_cleanup(MacroAssembler* masm) { Register tmp1 = R8_ARG6; @@ -1652,9 +1652,56 @@ static void continuation_enter_cleanup(MacroAssembler* masm) { #endif __ ld_ptr(tmp1, ContinuationEntry::parent_cont_fastpath_offset(), R1_SP); + __ st_ptr(tmp1, JavaThread::cont_fastpath_offset(), R16_thread); + + if (CheckJNICalls) { + // Check if this is a virtual thread continuation + Label L_skip_vthread_code; + __ lwz(R0, in_bytes(ContinuationEntry::flags_offset()), R1_SP); + __ cmpwi(CCR0, R0, 0); + __ beq(CCR0, L_skip_vthread_code); + + // If the held monitor count is > 0 and this vthread is terminating then + // it failed to release a JNI monitor. So we issue the same log message + // that JavaThread::exit does. + __ ld(R0, in_bytes(JavaThread::jni_monitor_count_offset()), R16_thread); + __ cmpdi(CCR0, R0, 0); + __ beq(CCR0, L_skip_vthread_code); + + // Save return value potentially containing the exception oop + Register ex_oop = R15_esp; // nonvolatile register + __ mr(ex_oop, R3_RET); + __ call_VM_leaf(CAST_FROM_FN_PTR(address, SharedRuntime::log_jni_monitor_still_held)); + // Restore potental return value + __ mr(R3_RET, ex_oop); + + // For vthreads we have to explicitly zero the JNI monitor count of the carrier + // on termination. The held count is implicitly zeroed below when we restore from + // the parent held count (which has to be zero). + __ li(tmp1, 0); + __ std(tmp1, in_bytes(JavaThread::jni_monitor_count_offset()), R16_thread); + + __ bind(L_skip_vthread_code); + } +#ifdef ASSERT + else { + // Check if this is a virtual thread continuation + Label L_skip_vthread_code; + __ lwz(R0, in_bytes(ContinuationEntry::flags_offset()), R1_SP); + __ cmpwi(CCR0, R0, 0); + __ beq(CCR0, L_skip_vthread_code); + + // See comment just above. If not checking JNI calls the JNI count is only + // needed for assertion checking. + __ li(tmp1, 0); + __ std(tmp1, in_bytes(JavaThread::jni_monitor_count_offset()), R16_thread); + + __ bind(L_skip_vthread_code); + } +#endif + __ ld(tmp2, in_bytes(ContinuationEntry::parent_held_monitor_count_offset()), R1_SP); __ ld_ptr(tmp3, ContinuationEntry::parent_offset(), R1_SP); - __ st_ptr(tmp1, JavaThread::cont_fastpath_offset(), R16_thread); __ std(tmp2, in_bytes(JavaThread::held_monitor_count_offset()), R16_thread); __ st_ptr(tmp3, JavaThread::cont_entry_offset(), R16_thread); DEBUG_ONLY(__ block_comment("} clean")); diff --git a/src/hotspot/cpu/riscv/sharedRuntime_riscv.cpp b/src/hotspot/cpu/riscv/sharedRuntime_riscv.cpp index b84e31b40a7..4f84ae256de 100644 --- a/src/hotspot/cpu/riscv/sharedRuntime_riscv.cpp +++ b/src/hotspot/cpu/riscv/sharedRuntime_riscv.cpp @@ -1,5 +1,5 @@ /* - * Copyright (c) 2003, 2023, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2003, 2024, Oracle and/or its affiliates. All rights reserved. * Copyright (c) 2014, 2020, Red Hat Inc. All rights reserved. * Copyright (c) 2020, 2023, Huawei Technologies Co., Ltd. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. @@ -905,6 +905,47 @@ static void continuation_enter_cleanup(MacroAssembler* masm) { __ ld(t0, Address(sp, ContinuationEntry::parent_cont_fastpath_offset())); __ sd(t0, Address(xthread, JavaThread::cont_fastpath_offset())); + + if (CheckJNICalls) { + // Check if this is a virtual thread continuation + Label L_skip_vthread_code; + __ lwu(t0, Address(sp, ContinuationEntry::flags_offset())); + __ beqz(t0, L_skip_vthread_code); + + // If the held monitor count is > 0 and this vthread is terminating then + // it failed to release a JNI monitor. So we issue the same log message + // that JavaThread::exit does. + __ ld(t0, Address(xthread, JavaThread::jni_monitor_count_offset())); + __ beqz(t0, L_skip_vthread_code); + + // Save return value potentially containing the exception oop in callee-saved x9 + __ mv(x9, x10); + __ call_VM_leaf(CAST_FROM_FN_PTR(address, SharedRuntime::log_jni_monitor_still_held)); + // Restore potential return value + __ mv(x10, x9); + + // For vthreads we have to explicitly zero the JNI monitor count of the carrier + // on termination. The held count is implicitly zeroed below when we restore from + // the parent held count (which has to be zero). + __ sd(zr, Address(xthread, JavaThread::jni_monitor_count_offset())); + + __ bind(L_skip_vthread_code); + } +#ifdef ASSERT + else { + // Check if this is a virtual thread continuation + Label L_skip_vthread_code; + __ lwu(t0, Address(sp, ContinuationEntry::flags_offset())); + __ beqz(t0, L_skip_vthread_code); + + // See comment just above. If not checking JNI calls the JNI count is only + // needed for assertion checking. + __ sd(zr, Address(xthread, JavaThread::jni_monitor_count_offset())); + + __ bind(L_skip_vthread_code); + } +#endif + __ ld(t0, Address(sp, ContinuationEntry::parent_held_monitor_count_offset())); __ sd(t0, Address(xthread, JavaThread::held_monitor_count_offset())); diff --git a/src/hotspot/cpu/x86/sharedRuntime_x86_64.cpp b/src/hotspot/cpu/x86/sharedRuntime_x86_64.cpp index 0c1dc865c78..31bbfb747f8 100644 --- a/src/hotspot/cpu/x86/sharedRuntime_x86_64.cpp +++ b/src/hotspot/cpu/x86/sharedRuntime_x86_64.cpp @@ -1354,9 +1354,48 @@ void static continuation_enter_cleanup(MacroAssembler* masm) { __ stop("Incorrect rsp at continuation_enter_cleanup"); __ bind(L_good_sp); #endif - __ movptr(rbx, Address(rsp, ContinuationEntry::parent_cont_fastpath_offset())); __ movptr(Address(r15_thread, JavaThread::cont_fastpath_offset()), rbx); + + if (CheckJNICalls) { + // Check if this is a virtual thread continuation + Label L_skip_vthread_code; + __ cmpl(Address(rsp, ContinuationEntry::flags_offset()), 0); + __ jcc(Assembler::equal, L_skip_vthread_code); + + // If the held monitor count is > 0 and this vthread is terminating then + // it failed to release a JNI monitor. So we issue the same log message + // that JavaThread::exit does. + __ cmpptr(Address(r15_thread, JavaThread::jni_monitor_count_offset()), 0); + __ jcc(Assembler::equal, L_skip_vthread_code); + + // rax may hold an exception oop, save it before the call + __ push(rax); + __ call_VM_leaf(CAST_FROM_FN_PTR(address, SharedRuntime::log_jni_monitor_still_held)); + __ pop(rax); + + // For vthreads we have to explicitly zero the JNI monitor count of the carrier + // on termination. The held count is implicitly zeroed below when we restore from + // the parent held count (which has to be zero). + __ movq(Address(r15_thread, JavaThread::jni_monitor_count_offset()), 0); + + __ bind(L_skip_vthread_code); + } +#ifdef ASSERT + else { + // Check if this is a virtual thread continuation + Label L_skip_vthread_code; + __ cmpl(Address(rsp, ContinuationEntry::flags_offset()), 0); + __ jcc(Assembler::equal, L_skip_vthread_code); + + // See comment just above. If not checking JNI calls the JNI count is only + // needed for assertion checking. + __ movq(Address(r15_thread, JavaThread::jni_monitor_count_offset()), 0); + + __ bind(L_skip_vthread_code); + } +#endif + __ movq(rbx, Address(rsp, ContinuationEntry::parent_held_monitor_count_offset())); __ movq(Address(r15_thread, JavaThread::held_monitor_count_offset()), rbx); @@ -3696,4 +3735,3 @@ void OptoRuntime::generate_exception_blob() { _exception_blob = ExceptionBlob::create(&buffer, oop_maps, SimpleRuntimeFrame::framesize >> 1); } #endif // COMPILER2 - diff --git a/src/hotspot/share/runtime/javaThread.cpp b/src/hotspot/share/runtime/javaThread.cpp index f6d514055b9..6687d85bcbd 100644 --- a/src/hotspot/share/runtime/javaThread.cpp +++ b/src/hotspot/share/runtime/javaThread.cpp @@ -910,19 +910,27 @@ void JavaThread::exit(bool destroy_vm, ExitType exit_type) { "should not have a Java frame when detaching or exiting"); ObjectSynchronizer::release_monitors_owned_by_thread(this); assert(!this->has_pending_exception(), "release_monitors should have cleared"); + // Check for monitor counts being out of sync. + assert(held_monitor_count() == jni_monitor_count(), + "held monitor count should be equal to jni: " INTX_FORMAT " != " INTX_FORMAT, + held_monitor_count(), jni_monitor_count()); + // All in-use monitors, including JNI-locked ones, should have been released above. + assert(held_monitor_count() == 0, "Failed to unlock " INTX_FORMAT " object monitors", + held_monitor_count()); + } else { + // Check for monitor counts being out of sync. + assert(held_monitor_count() == jni_monitor_count(), + "held monitor count should be equal to jni: " INTX_FORMAT " != " INTX_FORMAT, + held_monitor_count(), jni_monitor_count()); + // It is possible that a terminating thread failed to unlock monitors it locked + // via JNI so we don't assert the count is zero. } - // Since above code may not release JNI monitors and if someone forgot to do an - // JNI monitorexit, held count should be equal jni count. - // Consider scan all object monitor for this owner if JNI count > 0 (at least on detach). - assert(held_monitor_count() == jni_monitor_count(), - "held monitor count should be equal to jni: " INTX_FORMAT " != " INTX_FORMAT, - held_monitor_count(), jni_monitor_count()); if (CheckJNICalls && jni_monitor_count() > 0) { // We would like a fatal here, but due to we never checked this before there // is a lot of tests which breaks, even with an error log. log_debug(jni)("JavaThread %s (tid: " UINTX_FORMAT ") with Objects still locked by JNI MonitorEnter.", - exit_type == JavaThread::normal_exit ? "exiting" : "detaching", os::current_thread_id()); + exit_type == JavaThread::normal_exit ? "exiting" : "detaching", os::current_thread_id()); } // These things needs to be done while we are still a Java Thread. Make sure that thread @@ -960,9 +968,12 @@ void JavaThread::exit(bool destroy_vm, ExitType exit_type) { thread_name = os::strdup(name()); } - log_info(os, thread)("JavaThread %s (tid: " UINTX_FORMAT ").", - exit_type == JavaThread::normal_exit ? "exiting" : "detaching", - os::current_thread_id()); + if (log_is_enabled(Info, os, thread)) { + ResourceMark rm(this); + log_info(os, thread)("JavaThread %s (name: \"%s\", tid: " UINTX_FORMAT ").", + exit_type == JavaThread::normal_exit ? "exiting" : "detaching", + name(), os::current_thread_id()); + } if (log_is_enabled(Debug, os, thread, timer)) { _timer_exit_phase3.stop(); @@ -1990,17 +2001,23 @@ void JavaThread::trace_stack() { #endif // PRODUCT +// Slow-path increment of the held monitor counts. JNI locking is always +// this slow-path. void JavaThread::inc_held_monitor_count(intx i, bool jni) { #ifdef SUPPORT_MONITOR_COUNT - assert(_held_monitor_count >= 0, "Must always be greater than 0: " INTX_FORMAT, _held_monitor_count); + assert(_held_monitor_count >= 0, "Must always be non-negative: " INTX_FORMAT, _held_monitor_count); _held_monitor_count += i; if (jni) { - assert(_jni_monitor_count >= 0, "Must always be greater than 0: " INTX_FORMAT, _jni_monitor_count); + assert(_jni_monitor_count >= 0, "Must always be non-negative: " INTX_FORMAT, _jni_monitor_count); _jni_monitor_count += i; } + assert(_held_monitor_count >= _jni_monitor_count, "Monitor count discrepancy detected - held count " + INTX_FORMAT " is less than JNI count " INTX_FORMAT, _held_monitor_count, _jni_monitor_count); #endif } +// Slow-path decrement of the held monitor counts. JNI unlocking is always +// this slow-path. void JavaThread::dec_held_monitor_count(intx i, bool jni) { #ifdef SUPPORT_MONITOR_COUNT _held_monitor_count -= i; @@ -2009,6 +2026,12 @@ void JavaThread::dec_held_monitor_count(intx i, bool jni) { _jni_monitor_count -= i; assert(_jni_monitor_count >= 0, "Must always be greater than 0: " INTX_FORMAT, _jni_monitor_count); } + // When a thread is detaching with still owned JNI monitors, the logic that releases + // the monitors doesn't know to set the "jni" flag and so the counts can get out of sync. + // So we skip this assert if the thread is exiting. Once all monitors are unlocked the + // JNI count is directly set to zero. + assert(_held_monitor_count >= _jni_monitor_count || is_exiting(), "Monitor count discrepancy detected - held count " + INTX_FORMAT " is less than JNI count " INTX_FORMAT, _held_monitor_count, _jni_monitor_count); #endif } diff --git a/src/hotspot/share/runtime/javaThread.hpp b/src/hotspot/share/runtime/javaThread.hpp index 80d52eda390..52eae8eb067 100644 --- a/src/hotspot/share/runtime/javaThread.hpp +++ b/src/hotspot/share/runtime/javaThread.hpp @@ -811,6 +811,7 @@ private: static ByteSize cont_entry_offset() { return byte_offset_of(JavaThread, _cont_entry); } static ByteSize cont_fastpath_offset() { return byte_offset_of(JavaThread, _cont_fastpath); } static ByteSize held_monitor_count_offset() { return byte_offset_of(JavaThread, _held_monitor_count); } + static ByteSize jni_monitor_count_offset() { return byte_offset_of(JavaThread, _jni_monitor_count); } #if INCLUDE_JVMTI static ByteSize is_in_VTMS_transition_offset() { return byte_offset_of(JavaThread, _is_in_VTMS_transition); } diff --git a/src/hotspot/share/runtime/sharedRuntime.cpp b/src/hotspot/share/runtime/sharedRuntime.cpp index 2b06859c96d..3eb4c45ed1b 100644 --- a/src/hotspot/share/runtime/sharedRuntime.cpp +++ b/src/hotspot/share/runtime/sharedRuntime.cpp @@ -1931,6 +1931,20 @@ JRT_LEAF(void, SharedRuntime::complete_monitor_unlocking_C(oopDesc* obj, BasicLo SharedRuntime::monitor_exit_helper(obj, lock, current); JRT_END +// This is only called when CheckJNICalls is true, and only +// for virtual thread termination. +JRT_LEAF(void, SharedRuntime::log_jni_monitor_still_held()) + assert(CheckJNICalls, "Only call this when checking JNI usage"); + if (log_is_enabled(Debug, jni)) { + JavaThread* current = JavaThread::current(); + int64_t vthread_id = java_lang_Thread::thread_id(current->vthread()); + int64_t carrier_id = java_lang_Thread::thread_id(current->threadObj()); + log_debug(jni)("VirtualThread (tid: " INT64_FORMAT ", carrier id: " INT64_FORMAT + ") exiting with Objects still locked by JNI MonitorEnter.", + vthread_id, carrier_id); + } +JRT_END + #ifndef PRODUCT void SharedRuntime::print_statistics() { diff --git a/src/hotspot/share/runtime/sharedRuntime.hpp b/src/hotspot/share/runtime/sharedRuntime.hpp index d477929768f..93182a10126 100644 --- a/src/hotspot/share/runtime/sharedRuntime.hpp +++ b/src/hotspot/share/runtime/sharedRuntime.hpp @@ -349,6 +349,9 @@ class SharedRuntime: AllStatic { static void monitor_exit_helper(oopDesc* obj, BasicLock* lock, JavaThread* current); + // Issue UL warning for unlocked JNI monitor on virtual thread termination + static void log_jni_monitor_still_held(); + private: static Handle find_callee_info(Bytecodes::Code& bc, CallInfo& callinfo, TRAPS); static Handle find_callee_info_helper(vframeStream& vfst, Bytecodes::Code& bc, CallInfo& callinfo, TRAPS); diff --git a/test/hotspot/jtreg/runtime/vthread/JNIMonitor/JNIMonitor.java b/test/hotspot/jtreg/runtime/vthread/JNIMonitor/JNIMonitor.java index fc0fff0042d..5fe5aaf286f 100644 --- a/test/hotspot/jtreg/runtime/vthread/JNIMonitor/JNIMonitor.java +++ b/test/hotspot/jtreg/runtime/vthread/JNIMonitor/JNIMonitor.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2021, 2023, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2021, 2024, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -21,42 +21,275 @@ * questions. */ import jdk.test.lib.Asserts; +import jdk.test.lib.Utils; +import jdk.test.lib.process.ProcessTools; +import jdk.test.lib.process.OutputAnalyzer; +import java.lang.reflect.Constructor; +import java.lang.reflect.InvocationTargetException; +import java.util.List; import java.util.concurrent.atomic.AtomicReference; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executor; +import java.util.concurrent.Executors; +import java.util.concurrent.ThreadFactory; +import java.util.regex.Matcher; +import java.util.regex.Pattern; + +/* + * Tests that JNI monitors work correctly with virtual threads, + * There are multiple test scenarios that we check using unified logging output + * (both positive and negative tests). Each test case is handled by its own @-test + * definition so that we can run each sub-test independently. + * + * The original bug was only discovered because the ForkJoinPool worker thread terminated + * and trigerred an assertion failure. So we use a custom scheduler to give us control. + */ /** - * @test JNIMonitor - * @summary Tests that JNI monitors work correctly with virtual threads + * @test id=normal + * @bug 8327743 + * @summary Normal lock then unlock * @library /test/lib - * @compile JNIMonitor.java - * @run main/native/othervm JNIMonitor + * @modules java.base/java.lang:+open + * @requires vm.continuations + * @run driver JNIMonitor Normal + */ + +/** + * @test id=multiNormal + * @bug 8327743 + * @summary Normal lock then unlock by multiple threads + * @library /test/lib + * @modules java.base/java.lang:+open + * @requires vm.continuations + * @run driver JNIMonitor MultiNormal + */ + +/** + * @test id=missingUnlock + * @bug 8327743 + * @summary Don't do the unlock and exit normally + * @library /test/lib + * @modules java.base/java.lang:+open + * @requires vm.continuations + * @run driver JNIMonitor MissingUnlock + */ + +/** + * @test id=multiMissingUnlock + * @bug 8327743 + * @summary Don't do the unlock and exit normally, by multiple threads + * @library /test/lib + * @modules java.base/java.lang:+open + * @requires vm.continuations + * @run driver JNIMonitor MultiMissingUnlock + */ + +/** + * @test id=missingUnlockWithThrow + * @bug 8327743 + * @summary Don't do the unlock and exit by throwing + * @library /test/lib + * @modules java.base/java.lang:+open + * @requires vm.continuations + * @run driver JNIMonitor MissingUnlockWithThrow + */ + +/** + * @test id=multiMissingUnlockWithThrow + * @bug 8327743 + * @summary Don't do the unlock and exit by throwing, by multiple threads + * @library /test/lib + * @modules java.base/java.lang:+open + * @requires vm.continuations + * @run driver JNIMonitor MultiMissingUnlockWithThrow */ public class JNIMonitor { + public static void main(String[] args) throws Exception { + String test = args[0]; + String[] cmdArgs = new String[] { + "-Djava.library.path=" + Utils.TEST_NATIVE_PATH, + // Grant access to ThreadBuilders$VirtualThreadBuilder + "--add-opens=java.base/java.lang=ALL-UNNAMED", + // Enable the JNI warning + "-Xcheck:jni", + "-Xlog:jni=debug", + // Enable thread termination logging as a visual cross-check + "-Xlog:thread+os=info", + "JNIMonitor$" + test, + }; + OutputAnalyzer oa = ProcessTools.executeTestJava(cmdArgs); + oa.shouldHaveExitValue(0); + oa.stdoutShouldMatch(terminated); + + switch(test) { + case "Normal": + case "MultiNormal": + oa.stdoutShouldNotMatch(stillLocked); + break; + case "MissingUnlock": + oa.stdoutShouldMatch(stillLocked); + break; + case "MultiMissingUnlock": + parseOutputForPattern(oa.stdoutAsLines(), stillLocked, MULTI_THREAD_COUNT); + break; + case "MissingUnlockWithThrow": + oa.stdoutShouldMatch(stillLocked); + oa.stderrShouldContain(throwMsg); + break; + case "MultiMissingUnlockWithThrow": + parseOutputForPattern(oa.stdoutAsLines(), stillLocked, MULTI_THREAD_COUNT); + parseOutputForPattern(oa.stderrAsLines(), throwMsg, MULTI_THREAD_COUNT); + break; + + default: throw new Error("Unknown arg: " + args[0]); + } + oa.reportDiagnosticSummary(); + } + + // The number of threads for a multi tests. Arbitrarily chosen to be > 1 but small + // enough to not waste too much time. + static final int MULTI_THREAD_COUNT = 5; + + // The logging message for leaving a monitor JNI locked has the form + // [0.187s][debug][jni] VirtualThread (tid: 28, carrier id: 29) exiting with Objects still locked by JNI MonitorEnter. + // but if the test is run with other logging options then whitespace may get introduced in the + // log decorator sections, so ignore those. + static final String stillLocked = "VirtualThread \\(tid:.*exiting with Objects still locked by JNI MonitorEnter"; + // The carrier thread termination logging has the form: + // [1.394s][info][os,thread] JavaThread exiting (name: "pool-1-thread-1", tid: 3090592). + static final String terminated = "JavaThread exiting \\(name: \"pool-1-thread-1\""; + + static final String throwMsg = "Terminating via exception as requested"; + + // Check the process logging output for the given pattern to see if the expected number of + // lines are found. + private static void parseOutputForPattern(List lines, String pattern, int expected) { + Pattern p = Pattern.compile(pattern); + int found = 0; + for (String line : lines) { + Matcher m = p.matcher(line); + if (m.find()) { + found++; + } + } + if (found != expected) { + throw new RuntimeException("Checking for pattern \"" + pattern + "\": expected " + + expected + " but found " + found); + } + } + + // straight-forward interface to JNI monitor functions static native int monitorEnter(Object o); static native int monitorExit(Object o); - static { - System.loadLibrary("JNIMonitor"); - } - public static void main(String[] args) throws Throwable { - final Object monitor = new Object(); - final AtomicReference exception = new AtomicReference(); - Thread.ofVirtual().start(() -> { + // Isolate the native library loading to the actual test cases, not the class that + // jtreg Driver will load and execute. + static class TestBase { + + static { + System.loadLibrary("JNIMonitor"); + } + + // This gives us a way to control the scheduler used for our virtual threads. The test + // only works as intended when the virtual threads run on the same carrier thread (as + // that carrier maintains ownership of the monitor if the virtual thread fails to unlock it). + // The original issue was also only discovered due to the carrier thread terminating + // unexpectedly, so we can force that condition too by shutting down our custom scheduler. + private static Thread.Builder.OfVirtual virtualThreadBuilder(Executor scheduler) { + Thread.Builder.OfVirtual builder = Thread.ofVirtual(); try { - int res = monitorEnter(monitor); - Asserts.assertTrue(res == 0, "monitorEnter should return 0."); - Thread.yield(); - res = monitorExit(monitor); - Asserts.assertTrue(res == 0, "monitorExit should return 0."); - } catch (Throwable t) { - exception.set(t); + Class clazz = Class.forName("java.lang.ThreadBuilders$VirtualThreadBuilder"); + Constructor ctor = clazz.getDeclaredConstructor(Executor.class); + ctor.setAccessible(true); + return (Thread.Builder.OfVirtual) ctor.newInstance(scheduler); + } catch (InvocationTargetException e) { + Throwable cause = e.getCause(); + if (cause instanceof RuntimeException re) { + throw re; + } + throw new RuntimeException(e); + } catch (Exception e) { + throw new RuntimeException(e); } - }).join(); - if (exception.get() != null) { - throw exception.get(); + } + + static void runTest(int nThreads, boolean skipUnlock, boolean throwOnExit) throws Throwable { + final Object monitor = new Object(); + final AtomicReference exception = new AtomicReference(); + // Ensure all our VT's operate of the same carrier, sequentially. + ExecutorService scheduler = Executors.newSingleThreadExecutor(); + ThreadFactory factory = virtualThreadBuilder(scheduler).factory(); + for (int i = 0 ; i < nThreads; i++) { + Thread th = factory.newThread(() -> { + try { + int res = monitorEnter(monitor); + Asserts.assertTrue(res == 0, "monitorEnter should return 0."); + Asserts.assertTrue(Thread.holdsLock(monitor), "monitor should be owned"); + Thread.yield(); + if (!skipUnlock) { + res = monitorExit(monitor); + Asserts.assertTrue(res == 0, "monitorExit should return 0."); + Asserts.assertFalse(Thread.holdsLock(monitor), "monitor should be unowned"); + } + } catch (Throwable t) { + exception.set(t); + } + if (throwOnExit) { + throw new RuntimeException(throwMsg); + } + }); + th.start(); + th.join(); + if (exception.get() != null) { + throw exception.get(); + } + } + // Now force carrier thread to shutdown. + scheduler.shutdown(); } } + + // These are the actual test case classes that get exec'd. + + static class Normal extends TestBase { + public static void main(String[] args) throws Throwable { + runTest(1, false, false); + } + } + + static class MultiNormal extends TestBase { + public static void main(String[] args) throws Throwable { + runTest(MULTI_THREAD_COUNT, false, false); + } + } + + static class MissingUnlock extends TestBase { + public static void main(String[] args) throws Throwable { + runTest(1, true, false); + } + } + + static class MultiMissingUnlock extends TestBase { + public static void main(String[] args) throws Throwable { + runTest(MULTI_THREAD_COUNT, true, false); + } + } + + static class MissingUnlockWithThrow extends TestBase { + public static void main(String[] args) throws Throwable { + runTest(1, true, true); + } + } + + static class MultiMissingUnlockWithThrow extends TestBase { + public static void main(String[] args) throws Throwable { + runTest(MULTI_THREAD_COUNT, true, true); + } + } + } diff --git a/test/hotspot/jtreg/serviceability/jvmti/GetOwnedMonitorInfo/GetOwnedMonitorInfoTest.java b/test/hotspot/jtreg/serviceability/jvmti/GetOwnedMonitorInfo/GetOwnedMonitorInfoTest.java index 7b99104670f..80ca790a11d 100644 --- a/test/hotspot/jtreg/serviceability/jvmti/GetOwnedMonitorInfo/GetOwnedMonitorInfoTest.java +++ b/test/hotspot/jtreg/serviceability/jvmti/GetOwnedMonitorInfo/GetOwnedMonitorInfoTest.java @@ -73,14 +73,19 @@ public class GetOwnedMonitorInfoTest { runTest(false, false); } + static int t_num = 0; public static void runTest(boolean isVirtual, boolean jni) throws Exception { var threadFactory = isVirtual ? Thread.ofVirtual().factory() : Thread.ofPlatform().factory(); final GetOwnedMonitorInfoTest lock = new GetOwnedMonitorInfoTest(); Thread t1 = threadFactory.newThread(() -> { - Thread.currentThread().setName("Worker-Thread"); + Thread.currentThread().setName((isVirtual ? "Virtual-" : "") + "Worker-Thread-" + t_num); + t_num++; if (jni) { + System.out.println("Thread doing JNI call: " + + Thread.currentThread().getName()); + jniMonitorEnterAndLetObjectDie(); } @@ -92,7 +97,9 @@ public class GetOwnedMonitorInfoTest { // Make sure t1 contends on the monitor. synchronized (lock) { - System.out.println("Main starting worker thread."); + System.out.print("Main starting worker thread for "); + System.out.print(isVirtual ? "virtual " : "non-virtual "); + System.out.println(jni ? "JNI" : "non-JNI"); t1.start(); // Wait for the MonitorContendedEnter event