On Mon, 30 Jun 2025 11:24:28 GMT, Kevin Walls <kev...@openjdk.org> wrote:
>> ThreadDumper/ThreadSnapshot need to handle a failure to resolve the native >> VM JavaThread from a java.lang.Thread. This is hard to reproduce but a >> thread that has since terminated can provoke a crash. Recognise this and >> return a null ThreadSnapshot. > > Kevin Walls has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains 13 additional > commits since the last revision: > > - remove test definition changes > - TLH: use cv_internal_thread_to_JavaThread() > - Merge remote-tracking branch 'upstream/master' into 8359870_threadexited > - Test requires: permit linux debug testing > - comment update > - comment update > - newline > - Test fails on minimal VM: require jvmti feature > - Correct THROW macro > - ThreadDumper thread count > - ... and 3 more: https://git.openjdk.org/jdk/compare/393efb22...e2043438 Some minor nits/suggestions but generally looks good. src/hotspot/share/services/threadService.cpp line 1445: > 1443: JavaThread* java_thread = nullptr; > 1444: oop thread_oop; > 1445: bool has_javathread = tlh.cv_internal_thread_to_JavaThread(jthread, > &java_thread, &thread_oop); Suggestion: bool has_javathread = tlh.cv_internal_thread_to_JavaThread(jthread, &java_thread, &thread_oop); assert((has_javathread && thread_oop != nullptr) || !has_javathread, "Missing Thread oop"); src/hotspot/share/services/threadService.cpp line 1447: > 1445: bool has_javathread = tlh.cv_internal_thread_to_JavaThread(jthread, > &java_thread, &thread_oop); > 1446: Handle thread_h(THREAD, thread_oop); > 1447: bool is_virtual = java_lang_VirtualThread::is_instance(thread_h()); Suggestion: bool is_virtual = java_lang_VirtualThread::is_instance(thread_h()); // Deals with null Just to make it clear we don't need an external null check here. src/hotspot/share/services/threadService.cpp line 1450: > 1448: > 1449: if (!has_javathread && !is_virtual) { > 1450: return nullptr; // thread terminated Suggestion: return nullptr; // thread terminated so not of interest It took me a lot of backtracking in the Java code to find ThreadContainer which finally stated that only live threads are of interest, so an expanded comment here would help me. ( I was ready to suggest we should be creating a `ThreadSnapshot` that represents the terminated thread.) src/java.base/share/classes/jdk/internal/vm/ThreadDumper.java line 313: > 311: * @throws UncheckedIOException if an I/O error occurs > 312: */ > 313: private static boolean dumpThread(Thread thread, JsonWriter > jsonWriter) { Please document the return value in the javadoc comment. src/java.base/share/classes/jdk/internal/vm/ThreadSnapshot.java line 56: > 54: * Take a snapshot of a Thread to get all information about the > thread. > 55: * Return null if a ThreadSnapshot cannot be created, for example if > the > 56: * thread has terminated. Strictly speaking we can create a snapshot for a terminated thread, but our only client doesn't care about them so we don't. ------------- Marked as reviewed by dholmes (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/25958#pullrequestreview-2973312048 PR Review Comment: https://git.openjdk.org/jdk/pull/25958#discussion_r2176304440 PR Review Comment: https://git.openjdk.org/jdk/pull/25958#discussion_r2176303308 PR Review Comment: https://git.openjdk.org/jdk/pull/25958#discussion_r2176318110 PR Review Comment: https://git.openjdk.org/jdk/pull/25958#discussion_r2176319448 PR Review Comment: https://git.openjdk.org/jdk/pull/25958#discussion_r2176320553
