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

[simonis]

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" 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, 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 entry in another, unrelated class, that class will remain in an error state forever due to §5.4.3 Resolution of the Java Virtual Machine 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.". This means that the ClassCircularityError can repeatedly be thrown much later, in user code which is completely unrelated to class file transformation if that code happens to use the same class that failed to resolve a reference during instrumentation. A good example for this scenario are the sporadic ClassCircularityError that were seen in user code while using ConcurrentHashMaps caused by a change in the popular ByteBuddy library (see ByteBuddy #1666 for more details).

I'd therefor like to propose to add an @apiNote to j.l.i.ClassFileTransformer which makes users of the API aware of these potential issues:

 * @apiNote
 * If the invocation of {@link #transform transform()} for a class <code>C</code>
 * directly or transitively requires loading or resolving the same class <code>C</code>,
 * an error is thrown that is an instance of {@link LinkageError} (or a subclass).
 * Transforming core JDK classes or using libraries which depend on core JDK classes
 * during transformation increases the risk for such errors. If the {@link LinkageError}
 * occurs during reference resolution (see section 5.4.3 Resolution of <cite>The
 * Java Virtual Machine Specification</cite>) for a class <code>D</code>, the
 * corresponding reference resolution in class <code>D</code> will always fail
 * with the same error. This means that a {@link LinkageError} triggered during
 * transformation of <code>C</code> in a class <code>D</code> not directly related to
 * <code>C</code> can repeatedly occur later in arbitrary user code which uses class
 * <code>D</code>.

Please feel free to wordsmith the suggested @apiNote text :)

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8335619: Add an @apiNote to j.l.i.ClassFileTransformer to warn about recursive class loading and ClassCircularityErrors (Enhancement - P4)

Reviewers

• Thomas Stuefe (@tstuefe - Reviewer) 🔄 Re-review required (review applies to 080999e1)
• Chen Liang (@liach - Reviewer) 🔄 Re-review required (review applies to 080999e1)
• Alan Bateman (@AlanBateman - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/20011/head:pull/20011
$ git checkout pull/20011

Update a local copy of the PR:
$ git checkout pull/20011
$ git pull https://git.openjdk.org/jdk.git pull/20011/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 20011

View PR using the GUI difftool:
$ git pr show -t 20011

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/20011.diff

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back simonis! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@simonis This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

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

Reviewed-by: alanb, stuefe, liach

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 97 new commits pushed to the master branch:

• f677b90: 8267887: RMIConnector_NPETest.java fails after removal of RMI Activation (JDK-8267123)
• 1fe3ada: 8336284: Test TestClhsdbJstackLock.java/TestJhsdbJstackLock.java fails with -Xcomp after JDK-8335743
• c703d29: 8335710: serviceability/dcmd/vm/SystemDumpMapTest.java and SystemMapTest.java fail on Linux Alpine after 8322475
• 81a0d1b: 8325525: Create jtreg test case for JDK-8325203
• b3ef2a6: 8336036: Synthetic documentation for a record's equals is incorrect for floating-point types
• 687601e: 8336257: Additional tests in jmxremote/startstop to match on PID not app name
• 8890557: 8335623: Clean up HtmlTag.HtmlTag and make the ARIA role attribute global
• 73e3e0e: 8321509: False positive in get_trampoline fast path causes crash
• 9eb611e: 8334055: Unhelpful 'required: reference' diagnostics after JDK-8043226
• 5100303: 8335668: NumberFormat integer only parsing should throw exception for edge case
• ... and 87 more: https://git.openjdk.org/jdk/compare/6923a5114b2a9f02f0d6f0fefc21141ac3b9322a...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[openjdk]

@simonis The following label will be automatically applied to this pull request:

• serviceability

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 03: Full - Incremental (f8429bc4)
• 02: Full - Incremental (5fb85533)
• 01: Full - Incremental (080999e1)
• 00: Full (69b06d4b)

[tstuefe]

Small nit, otherwise ok.

As usual, excellent bug description.

[tstuefe]

LGTM

[simonis]

Notice that according to the CSR FAQ, I don't think that this change requires a CSR because it is not changing the specification but merely describes the actual behavior in some more detail:

Q: If the text of the javadoc of a public exported API is changing, is a CSR request needed?
A: A CSR request is required if the specification of a public exported API. Not all javadoc updates are specification changes. For example, typo fixes and rephrasings that do not alter the semantics of the API in question do not require CSR review.

[liach]

Looks good. We can always file a retroactive CSR if people find this clarification worth of archiving/recording.

[simonis]

@AlanBateman would you mind having a quick look at this trivial, documentation-only PR and let me know if you're OK with it?

Thank you and best regards,
Volker

[AlanBateman]

@AlanBateman would you mind having a quick look at this trivial, documentation-only PR and let me know if you're OK with it?

(Catching up as I was away for a few days).

I agree it would be useful to have an API note on this topic. There were several reports of deadlocks, ClassCircularityError, and other hard to diagnose issues when support for instrumentation was originally introduced in JDK 5. I think agent maintainers learned to exclude many of the core classes in java.base but that is always a moving target and there have been a few more sightings/reports recently (maybe due to newer features doing more in Java rather than in the VM).

I agree that the ClassFileTransformer class description is the best place for this. My initial reaction was to wonder about the j.l.instrument package description but it is more focused on starting agents and the JAR file attributes so I think your proposed location is best..

As regards the text then it might be better to start with a sentence to say that the "transform" method may be called from critical points in JDK core classes or called to transform JDK core classes. You've got some of this in second sentence but I think better to lead with it before revealing one of the several possible things that can happen. I think part of the lead in needs to say "great care must be taken" or something like that to get across the point that the ClassFileTransformer implementor has the potential to cause many possible issues. For the same class and linkage error then probably best to not use the phrase "transitively requires" as it's too close "requires transitive" used in module declarations.

Note that the @jvms tag can be used to reference the JVMS section. You'll see a few examples in other classes.

[simonis]

Thanks a lot for looking into this @AlanBateman. I've tried to integrate your suggestions into the new version of the PR.

I already used the @jvms tag in the latest version of my PR based on @liach's suggestion. I also updated the other reference to the JVMS above the new @apiNote which didn't used the tag either (and was wrong because the JVMS sections have changed since Java 5 :)

Please let me know if this sounds better now or if you see further room for improvement.

[AlanBateman]

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.

[simonis]

I haven't found anything either. The only specification-relevant mentioning of the issue I found is in the JVMTI Specification referenced at the beginning of the PR:

Care must be taken to avoid perturbing dependencies, especially when instrumenting core classes.

The example that follows describes an infinite recursion when instrumenting the the j.l.Object() constructor.

I think the exact reentrancy behavior isn't specified anywhere. Not even the exact that should be thrown in such a case is specified (see 8164165: JVM throws incorrect exception when ClassFileTransformer.transform() triggers class loading of class already being loaded for a discussion of different scenarios).

I think the real problem is that the JVMS predates the JVMTI specification and the interaction between instrumentation and class loading isn't clearly defined. I think it might even 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 the current 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 as I wrote, that predates JVMTI and when JVMTI was added, we missed the opportunity to specify its exact impact on class loading and resolution.

But all this is a much bigger discussion. Maybe we should open another issue for it?

[AlanBateman]

I've created JDK-8336296) for the spec issues.

[tstuefe]

Last version still looks good.

[simonis]

Thanks everybody for the quick and helpful reviews and @AlanBateman for opening JDK-8336296

[simonis]

/integrate

[openjdk]

Going to push as commit eec0e15.
Since your change was applied there have been 99 commits pushed to the master branch:

• 9b6f6c5: 8336082: Fix -Wzero-as-null-pointer-constant warnings in SimpleCompactHashtable
• 7a62032: 8336081: Fix -Wzero-as-null-pointer-constant warnings in JVMTypedFlagLimit ctors
• f677b90: 8267887: RMIConnector_NPETest.java fails after removal of RMI Activation (JDK-8267123)
• 1fe3ada: 8336284: Test TestClhsdbJstackLock.java/TestJhsdbJstackLock.java fails with -Xcomp after JDK-8335743
• c703d29: 8335710: serviceability/dcmd/vm/SystemDumpMapTest.java and SystemMapTest.java fail on Linux Alpine after 8322475
• 81a0d1b: 8325525: Create jtreg test case for JDK-8325203
• b3ef2a6: 8336036: Synthetic documentation for a record's equals is incorrect for floating-point types
• 687601e: 8336257: Additional tests in jmxremote/startstop to match on PID not app name
• 8890557: 8335623: Clean up HtmlTag.HtmlTag and make the ARIA role attribute global
• 73e3e0e: 8321509: False positive in get_trampoline fast path causes crash
• ... and 89 more: https://git.openjdk.org/jdk/compare/6923a5114b2a9f02f0d6f0fefc21141ac3b9322a...master

Your commit was automatically rebased without conflicts.

[openjdk]

@simonis Pushed as commit eec0e15.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.