Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
simonis wants to merge 4 commits into from
Closed

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

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
69b06d4
8335619: Add an @apiNote to j.l.i.ClassFileTransformer to warn about …
simonis Jul 3, 2024
080999e
Fixed text based on reviewer comments. Also added a '@jvms' tag to an…
simonis Jul 4, 2024
5fb8553
Addressed @AlanBateman's suggestions and updated copyright year
simonis Jul 9, 2024
f8429bc
Addressed @AlanBateman's new suggestions
simonis Jul 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 21 additions & 4 deletions src/java.instrument/share/classes/java/lang/instrument/ClassFileTransformer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2003, 2023, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2003, 2024, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand Down Expand Up @@ -155,11 +155,28 @@
* logging or debugging of format corruptions.
*
* <P>
* Note the term <i>class file</i> is used as defined in section 3.1 of
* <cite>The Java Virtual Machine Specification</cite>, to mean a
* sequence of bytes in class file format, whether or not they reside in a
* Note the term <i>class file</i> is used as defined in chapter {@jvms 4} The
* {@code class} File Format of <cite>The Java Virtual Machine Specification</cite>,
* to mean a sequence of bytes in class file format, whether or not they reside in a
* file.
*
* @apiNote
* Great care must be taken when transforming core JDK classes which are at the
* same time required during the transformation process as this can lead to class
* circularity or linkage errors.
*
* <P>
* If for example the invocation of {@link #transform transform()} for a class
* {@code C} requires loading or resolving the same class {@code C},
* an error is thrown that is an instance of {@link LinkageError} (or a subclass).
* If the {@link LinkageError} occurs during reference resolution (see section
* {@jvms 5.4.3} Resolution of <cite>The Java Virtual Machine Specification</cite>)
* for a class {@code D}, the resolution of the corresponding reference in class
* {@code D} will permanently fail with the same error at any subsequent attempt.
* This means that a {@link LinkageError} triggered during transformation of
* {@code C} in a class {@code D} not directly related to {@code C} can repeatedly
* occur later in arbitrary user code which uses {@code D}.
Copy link
Contributor

@AlanBateman AlanBateman Jul 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@simonis simonis Jul 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@AlanBateman AlanBateman Jul 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

*
* @see java.lang.instrument.Instrumentation
* @since 1.5
*/
Expand Down