JDK-8232091 : Concurrent class unloading with JVMTI
  • Type: Bug
  • Component: hotspot
  • Sub-Component: jvmti
  • Affected Version: 12,13,14
  • Priority: P4
  • Status: Closed
  • Resolution: Duplicate
  • Submitted: 2019-10-10
  • Updated: 2019-11-13
  • Resolved: 2019-11-12
Related Reports
Duplicate :  
JDK-8173658 - JvmtiExport::post_class_unload() is broken for non-JavaThread initiators
Description
In��hotspot/share/prims/jvmtiExport.cpp, method��JvmtiExport::post_class_unload:

We have this code:

assert(thread->is_VM_thread(), "wrong thread");

// get JavaThread for whom we are proxy
Thread *calling_thread = ((VMThread *)thread)>vm_operation()>calling_thread();
if (!calling_thread->is_Java_thread())

{ // cannot post an event to a non-JavaThread return; }

��

It seems problematic if we would try to post outside a safepoint.
Comments
Update: we set the type as "JVMTI_TYPE_CCHAR" and set the jvmtiParamKind as JVMTI_KIND_IN_PTR, to show it is a "char*".
13-11-2019
I wasn't sure if exposing the class name as a "char *" (rather than a jobject for the String) was appropriate for JVM TI events. But there are some precedents for using "char*" so that is fine.
13-11-2019
What do you mean we "can" make the changes? If we had existing users who were deadlocking or crashing by looking at the arguments, I'm not sure they would notice that now it returns a const char* and they can use the parameter.
12-11-2019
ClassUnload is not mentioned in the spec because it is an extension. The spec only refers to the extension mechanism. Removing/changing the ClassUnload event would be a behavioural change that could use a CSR request just for good measure. Though unlike other VMs it seems we never really advertised this event for actual end use. I'm not clear on whether we can make the changes to the event you suggest.
12-11-2019
Closing as duplicate of JDK-8173658
12-11-2019
Could we keep the ClassUnload extension event, but only deliver it from the ServiceThread as a deferred event. Also then only pass a char* for the klass name instead of the jobject for the threadObj and mirror? Does this need a CSR request to change the parameters of the ClassUnload extension event? I linked this to the existing JVMTI bug for this issue, so this is essentially a duplicate. I don't see any reference of the extended event for ClassUnload in the jvmti spec (I guess because it's an extension). Maybe my google searching is bad, but I see no references to this.
12-11-2019
Thanks for the information [~dcubed]! I think we can proceed to remove ClassUnload without any concern that someone may be trying to use it. The hardest part may be coming up with a replacement event. While there is obviously a desire to be able to test the mechanism as it is part of the JVM TI spec, introducing a fake event is not ideal as we are actually making that event part of our extension to JVM TI.
10-10-2019
The ClassUnload event only exists as a means to test the JVM/TI event extension mechanism. We'll need another "do nothing" event to keep that mechanism tested.
10-10-2019
I had not even considered what the jclass reference could be used for! The tests we have are minimal and only basically check that the event occurs and that, in one case, the class is non-NULL. As I said ClassUnload exists now only as an extension event - and it appears that the "extension event" mechanism was added at the same time that ClassUnload was removed from the spec (though the addition is missing from the spec change history!). I suspect this was to allow for a migration path for users who thought they wanted to continue to use this event. It's possible there are people using it just for accounting purposes for monitoring class unloading. I agree we should look into getting rid of this.
10-10-2019
Moreover, Robbin and I can't find any JVMTI documentation that says this event exists at all. I wonder if we can just remove this code as it's not documented to exist, and could never really have done anything useful if one tried to use it. Or maybe I'm missing something?
10-10-2019
I tried my hypothesis that it could never have made a difference what jthread/jclass values we pass in to the callback, since we are in a context where we can't really do anything useful in the callback that talks to the JVM without deadlocking the whole VM. So instead of grabbing the requesting thread, I extract the JNI environment of a random Java thread instead, in my case, the ServiceThread, and pass in os::random() as jthread and jclass in the callback. Tests using this event seem to be equally as happy about that, because it is indeed equivalent to what we have today in terms of parameter usefulness.
10-10-2019
I'm also a bit suspicious of this code. We grab the dead class mirror, stick it in a handle, and pass that handle into the callback, as a way of identifying what has died. But surely, you can't do anything with that jclass? Not just because it's dead and doing things with it would be extremely dangerous, but also because any API we have for using either determining the identity of this jclass or trying to in any way read or write any value from/to it, would go through the JNI API, which would deadlock the VM. Because we are in a safepoint, and the transitioning code of any JNI call operating on the jclass, extracts the "fake" JavaThread* from the jni environment and tries to block on the safepoint with that thread, as we are in a safepoint. I don't think there is any way that would ever have worked. So we might as well have sent os::random() as jclass, for any valid existing user of this API. And that doesn't seem all that useful to me.
10-10-2019
There has to be a JavaThread JNIEnv available for invoking the callback. That said ... Overall though the existing code looks very suspicious to me. We use the JNIEnv of the "real thread" when executing the callback, but it is still the VMThread that is executing that code so the callback has to be very constrained in what it actually executes! If it tries to execute Java code, for example, then I would expect us to crash and burn. Also note this is an implementation specific "extension event". Our tests only have trivial ClassUnload callbacks defined. I also noticed that this assert is incorrect: assert(((Thread *)real_thread)->is_ConcurrentGC_thread() || (real_thread->is_Java_thread() && prev_state == _thread_blocked), "should be ConcurrentGCThread or JavaThread at safepoint"); as we've already established we are dealing with a JavaThread. This is apparently a CMS leftover. I'll also note that this code basically hijacks whatever JavaThread may have initiated the current VM operation, even though that thread and the operation may have nothing to do with the class unloading. Seems highly fragile, and of course depends on the proxy JavaThread being blocked for the duration of the callback.
10-10-2019
Further digging shows that this event was removed from the spec with https://bugs.openjdk.java.net/browse/JDK-4898113 Maybe it's about time it's also removed from the code...?
10-10-2019