On Sat, 10 Jun 2023 05:16:28 GMT, Jaikiran Pai <j...@openjdk.org> wrote:
>> Can I please get a review of this change which proposes to fix the issue >> noted in https://bugs.openjdk.org/browse/JDK-8308184? >> >> When an application is launched, the `app` classloader internally uses a >> `jdk.internal.loader.URLClassPath` to find and load the main class being >> launched. The `URLClassPath` uses a list of classpath entries to find >> resources. Depending on the classpath entry, the `URLClassPath` will use a >> relevant loader. A couple of such loaders are `URLClassPath$FileLoader` (for >> loading resources from directories) and `URLClassPath$JarLoader` (for >> loading resources from jar files). >> >> `JarLoader` creates instances of `java.net.URL` to represent the jar file >> being loaded. `java.net.URL` uses protocol specific >> `java.net.URLStreamHandler` instance to handle connections to those URLs. >> When constructing an instance of `URL`, callers can pass a protocol handler. >> If it is not passed then the `URL` class looks for protocol handlers that >> might have been configured by the application. The >> `java.protocol.handler.pkgs` system property is the one which allows >> overriding the protocol handlers (even for the `jar` protocol). When this >> property is set, the `URL` class triggers lookup and classloading of the >> protocol handler classes. >> >> The issue that is reported is triggered when the >> `java.protocol.handler.pkgs` system property is set and the classpath has >> too many jar files. `app` classloader triggers lookup of the main class and >> the `URLClassPath` picks up the first entry in the classpath and uses a >> `JarLoader` (in this example our classpath entries have a jar file at the >> beginning of the list). The `JarLoader` instantiates a `java.net.URL`, which >> notices that the `java.protocol.handler.pkgs` is set, so it now triggers >> lookup of a (different) class using the same classloader and thus the same >> `URLClassPath`. The `URLClassPath` picks the next classpath entry and then >> calls into the `URL` again through the `JarLoader`. This sequence ends up >> being re-entrant calls and given the large number of classpath entries, >> these re-entrant calls end up with a `StackOverflowError` as shown in the >> linked JBS issue. >> >> The commit in this PR fixes this issue by using the system provided protocol >> handler implementation of the `jar` protocol in the `app` classloader. This >> results in the `URL` instances created through the `JarLoader` to use this >> specific handler instance. This allows the `app` classloader which is >> responsible for loading the application's main class ... > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > minor update to the test to allow tracking the time taken to create the > jars for debug purpose Thanks for taking this one. My guess is that this issue goes back 20+ but not noticed because it's running with the system property to specify another location for protocol handlers is probably rare, and it seems a scanning of a large large class path to trigger it. I assume you'll see the comment about moving the test to be with the other tests for URLClassPath. src/java.base/share/classes/jdk/internal/loader/URLClassPath.java line 212: > 210: // the application class loader uses the built-in protocol > handler to avoid protocol > 211: // handler lookup when opening JAR files on the class path. > 212: this.jarHandler = new sun.net.www.protocol.jar.Handler(); Good, this looks much better. test/jdk/java/net/URL/HandlersPkgPrefix/LargeClasspathWithPkgPrefix.java line 44: > 42: * @run driver LargeClasspathWithPkgPrefix > 43: */ > 44: public class LargeClasspathWithPkgPrefix { test/jdk/java/net/URL/HandlersPkgPrefix is an unusual location for the test, maybe it could move to test/jdk/sun/misc/URLClassPath so it's the same location as the other tests for URLClassPath. Separately (not suggesting it for this PR) is that test directory should probably be renamed to jdk/internal/loader as the URLClassPath moved in JDK 9. ------------- Marked as reviewed by alanb (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14395#pullrequestreview-1473325189 PR Review Comment: https://git.openjdk.org/jdk/pull/14395#discussion_r1225140617 PR Review Comment: https://git.openjdk.org/jdk/pull/14395#discussion_r1225140504
