RFR: 8355631: The events might be generated after VM_DEATH event [v3]

[David Holmes]

On Sat, 25 Oct 2025 18:27:39 GMT, Leonid Mesnik <lmesnik at openjdk.org> 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