On Thu, 29 May 2025 14:19:08 GMT, Jaikiran Pai <j...@openjdk.org> wrote:
>> Can I please get a review of this doc-only change which proposes to add an >> `@apiNote` on the constructors of `URLClassLoader` and `SecureClassLoader` >> to explain the current implementation of these constructors? This addresses >> https://bugs.openjdk.org/browse/JDK-8228773? >> >> As noted in that issue, this updated documentation is to help applications >> be aware that a `null` value which represents the bootstrap class loader >> when passed for `parent` class loader will mean that the constructed >> `URLClassLoader` may not be able to load all platform classes. >> >> Specifically, only a specific set of modules are mapped (at JDK build time) >> to the bootstrap class loader. Some modules like `java.sql` are visible to >> the platform class loader but not to the bootstrap classloader. The >> distinction between these class loaders is explained in the API >> documentation of `ClassLoader` class >> https://docs.oracle.com/en/java/javase/24/docs/api/java.base/java/lang/ClassLoader.html#builtinLoaders. >> >> Using `null` (which represents a bootstrap class loader) for `parent` when >> constructing the `URLClassLoader` then means that the class loader would not >> be able to load some of these platform classes. For example, consider: >> >> >> import java.net.*; >> >> public class Test { >> public static void main(String[] args) throws Exception { >> System.out.println("testing against Java " + >> System.getProperty("java.version")); >> >> final ClassLoader cl = new URLClassLoader(new URL[0], null); >> final Class<?> klass = cl.loadClass("java.sql.ResultSet"); // load a >> platform class that belongs to the java.sql module >> System.out.println("loaded " + klass + " using classloader: " + >> klass.getClassLoader()); >> >> } >> } >> >> which constructs the `URLClassLoader` with the bootstrap class loader as its >> parent and then attempts to load a platform class `java.sql.ResultSet`. >> Since this class' module is mapped to the platform class loader and not the >> bootstrap class loader, running this code against Java 9+ will result in a >> `ClassNotFoundException`: >> >> >> testing against Java 24 >> Exception in thread "main" java.lang.ClassNotFoundException: >> java.sql.ResultSet >> at java.base/java.net.URLClassLoader.findClass(URLClassLoader.java:349) >> at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:557) >> at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:490) >> at Test.main(Test.java:8) >> >> >> No new tests have been introduced and existing tes... > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > additional sentence to the apiNote src/java.base/share/classes/java/net/URLClassLoader.java line 87: > 85: * > 86: * @param urls the URLs from which to load classes and resources > 87: * @param parent the parent class loader for delegation, can be > null s/null/{@code null}/ src/java.base/share/classes/java/net/URLClassLoader.java line 123: > 121: * obtain protocol handlers when creating new jar URLs. > 122: * > 123: * @apiNote If the {@code parent} is specified as {@code null} (for > the I think "If {@code parent}" is more correct, since you are implicitly referring to the parameter. src/java.base/share/classes/java/net/URLClassLoader.java line 127: > 125: * classes are visible. > 126: * See {@linkplain ClassLoader##builtinLoaders Run-time Built-in > Class Loaders} > 127: * for information on the system class loader and other built-in > class loaders. Seems more relevant to say "for information on the bootstrap class loader and other built-in class loaders." since you specifically mention bootstrap loader in the first sentence. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/25517#discussion_r2114361908 PR Review Comment: https://git.openjdk.org/jdk/pull/25517#discussion_r2114370688 PR Review Comment: https://git.openjdk.org/jdk/pull/25517#discussion_r2114366997
