JDK-8164165 : JVM throws incorrect exception when ClassFileTransformer.transform() triggers class loading of class already being loaded
  • Type: Bug
  • Component: hotspot
  • Sub-Component: jvmti
  • Affected Version: 9,10,11
  • Priority: P4
  • Status: Open
  • Resolution: Unresolved
  • Submitted: 2016-08-16
  • Updated: 2024-07-17
The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.
Other
tbdUnresolved
Related Reports
Relates :  
Relates :  
Relates :  
Relates :  
Relates :  
Relates :  
Relates :  
Description
JDK-8160849 uncovered some ugliness in the ClassFileTransformer.transform() support when transform() makes a reference (directly or indirectly) to the class being transformed. This results in a "circular" reference, although not circular in the ClassCircularityError sense, which happens when there is circularity in the class hierarchy. The JVM currently throws CCE in some cases (which should not be thrown unless the class hierarchy is circular), or throws a LinkageError, which is probably the correct exception, but has a misleading message. More details on the JVMs error handling is given below.

The JLI spec says nothing about what happens if the transform() method triggers additional class loading, possibly referencing the the class being transformed.

https://docs.oracle.com/javase/8/docs/api/java/lang/instrument/ClassFileTransformer.html

The spec should probably should clarify this situation. These circular references put the JVM in a difficult position, and probably the best it can do is throw some meaningful exception.

Note that JLI does prevent recursive invocation of transform(). See the call to tryToAcquireReentrancyToken in jdk/src/java.instrument/share/native/libinstrument/JPLISAgent.c, which disables recursive invocation of transform(). I think it's purpose might be to avoid the CCE situation, but it's not 100% safe. 

Attached to this CR is a test case which demonstrates the JVM behavior with 3 separate "circular" scenarios, each which the JVM handles differently. The test is not written to detect incorrect behavior and. It simply triggers the circular references, and prints out the exceptions thrown by the JVM. Below is a description of the 3 failures it triggers:

Scenario #1: The loading of a classpath class is triggered. When the JVM attempts to load  its superclass, transform() is called for the the superclass. From transform() a reference to the subclass is made. The JVM will try to load the subclass because it is not yet fully loaded. This results in a ClassCircularityError when the JVM sees it is already loading this class. This exception is invalid since ClassCircularityError should only be used when there is a circularity in the class hierarchy, which is not the case here. A LinkageError is likely the correct exception.

Note that the CCE for this test case is thrown by SystemDictionary::resolve_super_or_fail():

      PlaceholderEntry* probe = placeholders()->get_entry(p_index, p_hash, child_name, loader_data);
      if (probe && probe->check_seen_thread(THREAD, PlaceholderTable::LOAD_SUPER)) {
          throw_circularity_error = true;
      }

Scenario #2: The loading of a classpath class is triggered. When the JVM attempts to load the class, transform() is called for it. From transform() a reference to the class is made. The JVM will try to load the class because it is not yet fully loaded. This results in a LinkageErorr. This is probably the correct exception to throw. However, the message with the LinkageErorr states "attempted duplicate class definition", which is misleading. Also, the reference to the class from transform() actually succeeds, resulting in the class being fully loaded at that time. The LinkageErorr is not thrown until transform() returns and the initial loading of the class resumes. This means the initial attempt to load the class fails rather than the attempt made by the transform() method. This seems incorrect. since it is transform() that is behaving incorrectly, not the initial attempt to load the class. It also leaves you with the situation where if the initial attempt to load the class is reattempted, it will succeed because the class is already fully loaded by then.

Note the LinkageError is thrown by SystemDictionary::check_constraints() when it determines that the given class has already been loaded. This is another indication that this scenario is not being handled properly, since the "duplicate" class here has nothing to do with loader constrains. It has to do with class loading code not recognizing that transform() triggered loading the class it was currently loading (or probably more correctly that transform() was able to trigger loading the class without getting an error).

Scenario #3: Same as (2) except the class is on the bootclasspath. The JVM handles this differently, and it results in ClassCircularityError being thrown when transform() references the class. An exception is appropriate here, but just as with (1), it probably should be LinkageErorr, not ClassCircularityError. The CCE is detected by the following code in SystemDictionary::resolve_instance_class_or_null():

          if (oldprobe->check_seen_thread(THREAD, PlaceholderTable::LOAD_INSTANCE)) {
            throw_circularity_error = true;
          } else {

Comments
This actually varies whether the class loader is parallel capable or not. If it's parallel capable, we allow the loading to continue. There's a test: serviceability/jvmti/RedefineClasses/TransformerDeadlockTest.java I would not want to see any redefinition hacks in class loading, although there is already a is_being_redefined flag.
18-06-2024

We run into this problem as well when instrumenting core library classes. I agree that `ClassCircularityError` might not be the correct exception to throw, but more concerning for me is the fact that in this scenario, if the resolution of a field fails due to a `ClassCircularityError` in code that is called from `transform()` during instrumentation, this will leave the corresponding field in an unresolved state forever (i.e. JVM_CONSTANT_UnresolvedClassInError). This can lead to subsequent `ClassCircularityError` being thrown from unrelated user code long after the transformation failed. In our case an application tried to instrument `ThreadLocalRandom` and during `transform()` it used a `ConcurrentHashMap` which in turn uses `ThreadLocalRandom` in some cases. The transformation failed with a `ClassCircularityError` (which is fine) but also left `ConcurrentHashMap` in an inconsistent state. Much later, if the application uses `ConcurrentHashMap` in such a way that it requires `ThreadLocalRandom`, it will always fail with a `ClassCircularityError` which can not be easily attributed to the previous, failed transformation by the user. So I wonder if it would be possible to treat class loading errors during transformation differently, such that they will not lead to a permanent resolution error for the corresponding constant pool entries. I know that this will violate section ยง 5.4.3 Resolution (https://docs.oracle.com/javase/specs/jvms/se8/html/jvms-5.html#jvms-5.4.3) of the JVM specification which mandates that "if an attempt by the Java Virtual Machine to resolve a symbolic reference fails because an error is thrown that is an instance of LinkageError (or a subclass), then subsequent attempts to resolve the reference always fail with the same error that was thrown as a result of the initial resolution attempt". But the JVMS predates the JVMTI specification and as it was mentioned in this thread already earlier, the interaction between instrumentation and class loading isn't clearly defined. Do you think it would be possible to introduce something like an "is-being-transformed flag" as proposed by [~dholmes] in order to change resolution errors during transformation into transient errors which still can be recovered later, after transformation finished?
18-06-2024

[~dholmes] I'm not sure how much this will help. Some sort of exception needs to be thrown because the class is already being loaded by the thread. Right we normally see a CCE, but as described above, sometimes other exceptions. I'm seeing this issue again in JDK-8284777. The explanation in the following comment is useful background info: https://bugs.openjdk.java.net/browse/JDK-8284777?focusedCommentId=14488501&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14488501 What it boils down to is once there is an exception due to the recursive class load attempt, the constant pool entry gets tagged with JVM_CONSTANT_UnresolvedClassInError, causing all future references to the same entry to throw the same exception. I don't see how we can get around throwing some sort of exception. The transform() method made a reference to a class that is the same as the class currently be transformed(). Even if class loading code detects this, what can it do to correct the situation in a way that allows the class to be loaded and the transform() method to not to end up with an exception thrown to it? Since the reference is likely an indirect one, and one the transform() method is not aware of, it can't really defend against it other than by being written to not make the reference in the first place. The transform() method also needs hope that class library changes in the future don't create this CCE situation, which is the root cause of JDK-8284777, and is due to library changes made for Loom.
12-04-2022

> [~cjplummer] I was thinking that we avoid the throwing of the exception by not actually trying to load if the load is already in progress - somewhat similar to how we don't enforce that a "class must be initialized" if the initialization is already underway by the current thread. I don't know if this is actually feasible for this case as I'm not clear on the exact context where the class being transformed needs to be recursively loaded again. That might work, but I don't know if the class is actually in a usable state when the class circularity is detected. Basically this means it has to be usable by the time time transform() is called. I think it is not, because transform() is called after the bytes are loaded so they can be modified, so I doubt there is a usual InstanceKlass at this point.
12-04-2022

[~cjplummer] I was thinking that we avoid the throwing of the exception by not actually trying to load if the load is already in progress - somewhat similar to how we don't enforce that a "class must be initialized" if the initialization is already underway by the current thread. I don't know if this is actually feasible for this case as I'm not clear on the exact context where the class being transformed needs to be recursively loaded again.
12-04-2022

It sounds to me like we need an is-being-transformed flag on the instanceKlass that we can check if we hit classloading logic for the same class.
25-05-2021