RFR: 8335619: Add an @apiNote to j.l.i.ClassFileTransformer to warn about recursive class loading and ClassCircularityErrors [v3]

[Alan Bateman]

On Tue, 9 Jul 2024 14:18:53 GMT, Volker Simonis <simonis at openjdk.org> wrote:

>> Since Java 5 the `java.lang.instrument` package provides services that allow Java programming language agents to instrument (i.e. modify the bytecode) of programs running on the Java Virtual Machine. The `java.lang.instrument` functionality is based and implemented on top of the native Java Virtual Machine Tool Interface (JVMTI) also introduced in Java 5. But because the `java.lang.instrument` API is a pure Java API and uses Java classes to instrument Java classes it imposes some usage restrictions which are not very well documented in its API specification.
>> 
>> E.g. the section on ["Bytecode Instrumentation"](https://docs.oracle.com/en/java/javase/21/docs/specs/jvmti.html#bci) in the JVMTI specification explicitly warns that special "*Care must be taken to avoid perturbing dependencies, especially when instrumenting core classes*". The risk of such "perturbing dependencies" is obviously much higher in a Java API like `java.lang.instrument`, but a more detailed explanation and warning is missing from its API documentation.
>> 
>> The most evident class file transformation restriction is that while a class A is being loaded and transformed it is not possible to use this same class directly or transitively from the `ClassFileTransformer::transform()` method. Violating this rule will result in a `ClassCircularityError` (the exact error type is disputable as can be seen in [8164165: JVM throws incorrect exception when ClassFileTransformer.transform() triggers class loading of class already being loaded](https://bugs.openjdk.org/browse/JDK-8164165), but the result would be a `LinkageError in any case).
>> 
>> The risk to run into such a `ClassCircularityError` error increases with the amount of code a transforming agent is transitively using from the `transform()` method. Using popular libraries like ASM, ByteBuddy, etc. for transformation further increases the probability of running into such issues, especially if the agent aims to transform core JDK library classes.
>> 
>> By default, the occurrence of a `ClassCircularityError` in `ClassFileTransformer::transform()` will be handled gracefully with the only consequence that the current transformation target will be loaded unmodified (see `ClassFileTransformer` API spec: "*throwing an exception has the same effect as returning null*"). But unfortunately, it can also have a subtle but at the same time much more far-reaching consequence. If the `ClassCircularityError` occurs during the resolution of a constant pool ...
>
> Volker Simonis has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Addressed @AlanBateman's suggestions and updated copyright year

src/java.instrument/share/classes/java/lang/instrument/ClassFileTransformer.java line 158:

> 156:  *
> 157:  * <P>
> 158:  * Note the term <i>class file</i> is used as defined in section {@jvms 4} The

The existing sentence uses "section" but I assume it should be "Chapter".

src/java.instrument/share/classes/java/lang/instrument/ClassFileTransformer.java line 167:

> 165:  * same time required during the transformation process as this can lead to class
> 166:  * circularity or linkage errors. Using bytecode transformation libraries which depend
> 167:  * on core JDK class can increase the risk of such errors.

It's probably impossible to create a BCI library that don't depend on JDK core classes so maybe it would be better to drop the second sentence.

src/java.instrument/share/classes/java/lang/instrument/ClassFileTransformer.java line 179:

> 177:  * This means that a {@link LinkageError} triggered during transformation of
> 178:  * {@code C} in a class {@code D} not directly related to {@code C} can repeatedly
> 179:  * occur later in arbitrary user code which uses {@code D}.

This paragraph looks okay but I can't help thinking we should have something in normative text to reference that specifies the reentrancy behavior.  Maybe I missed it but I thought we have something in the API docs on this.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/20011#discussion_r1672215548
PR Review Comment: https://git.openjdk.org/jdk/pull/20011#discussion_r1672219986
PR Review Comment: https://git.openjdk.org/jdk/pull/20011#discussion_r1672255885