On Sat, 25 Oct 2025 18:27:39 GMT, Leonid Mesnik <[email protected]> wrote:
>> The JVMTI spec says: >> https://docs.oracle.com/en/java/javase/24/docs/specs/jvmti.html#VMDeath >> `The VM death event notifies the agent of the termination of the VM. No >> events will occur after the VMDeath event.` >> >> However, current implementation changes state and only after this start >> disabling events. >> >> It might be not a conformance issue, because there is no way to get thread >> state in the very beginning of event. >> The main practical issue is that currently certain events are generated when >> VM is becoming dead. So any function in event should check error against >> JVMTI_PHASE_DEAD. We can easily trigger it by running tests with enabled >> https://bugs.openjdk.org/browse/JDK-8352654 >> >> Also, it would be useful to guarantee that VM_DEATH is last event so users >> can safely close/destroy all supported all structures used by Jvmti agent >> (like RawMonitors). >> >> The proposed fix is to stop events posting and wait for already executing >> events before vm_death is posted. >> >> Currently, I haven't seen problems with this fix and >> https://bugs.openjdk.org/browse/JDK-8352654. > > Leonid Mesnik has updated the pull request incrementally with one additional > commit since the last revision: > > cleanup This seems like a good approach for dealing with the problem. A couple of minor suggestions. src/hotspot/share/prims/jvmtiEventController.cpp line 38: > 36: #include "runtime/frame.inline.hpp" > 37: #include "runtime/javaThread.inline.hpp" > 38: #include "runtime/serviceThread.hpp" Why do we need this? src/hotspot/share/prims/jvmtiEventController.cpp line 1229: > 1227: } > 1228: > 1229: // Some events might be still in callback for daemons threads and > ServiceThread. Suggestion: // Some events might be still in callback for daemon threads and ServiceThread. src/hotspot/share/prims/jvmtiEventController.cpp line 1230: > 1228: > 1229: // Some events might be still in callback for daemons threads and > ServiceThread. > 1230: const double start = os::elapsedTime(); Given we sleep for 100ms each iteration we could simply count the iterations and break at 600, rather than tracking elapsed time. Not that it makes any real difference. src/hotspot/share/prims/jvmtiEventController.cpp line 1232: > 1230: const double start = os::elapsedTime(); > 1231: const double max_wait_time = 60; > 1232: while (in_callback_count() > 0) { Suggestion: // The first time we see the callback count reach zero we know all actual callbacks are complete. The count // could rise again, but those "callbacks" will immediately see `execution_finished()` and return (dropping the count). while (in_callback_count() > 0) { src/hotspot/share/prims/jvmtiEventController.hpp line 203: > 201: // wait until already executing callbacks are finished. > 202: volatile static bool _execution_finished; > 203: volatile static int _in_callback_count; Suggestion: volatile static bool _execution_finished; volatile static int _in_callback_count; src/hotspot/share/prims/jvmtiExport.cpp line 797: > 795: JavaThread *thread = JavaThread::current(); > 796: JvmtiEventMark jem(thread); > 797: // JVMTI_JAVA_EVENT_CALLBACK_BLOCK shouldn't be used here Suggestion: // JVMTI_JAVA_EVENT_CALLBACK_BLOCK must not be used here ------------- Marked as reviewed by dholmes (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/27504#pullrequestreview-3381836643 PR Review Comment: https://git.openjdk.org/jdk/pull/27504#discussion_r2464461016 PR Review Comment: https://git.openjdk.org/jdk/pull/27504#discussion_r2464465508 PR Review Comment: https://git.openjdk.org/jdk/pull/27504#discussion_r2464487442 PR Review Comment: https://git.openjdk.org/jdk/pull/27504#discussion_r2464485527 PR Review Comment: https://git.openjdk.org/jdk/pull/27504#discussion_r2464451942 PR Review Comment: https://git.openjdk.org/jdk/pull/27504#discussion_r2464471886
