On Fri, 23 Aug 2024 10:45:49 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-8338445? > > The JDK internal class `jdk.internal.loader.URLClassPath` is used by > classloader implementations in the JDK (for example by the > `java.net.URLClassLoader` and the > `jdk.internal.loader.ClassLoaders$AppClassLoader`). The implementation in > `URLClassPath` constructs internal `Loader`s for each URL that is in the > classpath of that instance. `Loader` implementations hold on to underlying > resources from which the classpath resources are served by the `URLClassPath`. > > When constructing a `Loader`, the `URLClassPath` allows the loader > implementation to parse a local classpath for that specific `Loader`. For > example, the `JarLoader` (an internal class of the JDK) will parse > `Class-Path` attribute in the jar file's manifest to construct additional > URLs that will be included in the classpath through which resources will be > served by `URLClassPath`. While parsing the local classpath, if the `Loader` > instance runs into any `IOException` or a `SecurityException`, the > `URLClassPath` will ignore that specific instance of the `Loader` and will > not hold on to it as a classpath element. > > As noted earlier, `Loader` instances may hold onto underlying resources. When > the `URLClassPath` ignores such `Loader`(s) and doesn't call `close()` on > them, then that results in potential resource leaks. The linked JBS issue > demonstrates a case where the `JarFile` held by the `JarLoader` doesn't get > closed (until GC garbage collects the `JarLoader` and thus the `JarFile`), > when the `URLClassPath` ignores that `JarLoader` due to an `IOException` when > the `JarLoader` is parsing the `Class-Path` value from the jar's manifest. > > The commit in this PR addresses the issue by calling `close()` on such > `Loader`s that get rejected by the `URLClassPath` when either an > `IOException` or a `SecurityException` gets thrown when constructing the > `Loader` or parsing the local classpath. > > A new jtreg test has been introduced which consistently reproduces this issue > (on Windows) and verifies the fix. Additionally tier1, tier2 and tier3 > testing has completed with this change without any failures. The change looks okay, I suspect the issue with handling malformed Class-Path values has been there for 20+ years but hasn't come up. For the bug report then it would be interesting to know if there is a plugin in the eco system that is creating the bad values or whether it's just a one-off. src/java.base/share/classes/jdk/internal/loader/URLClassPath.java line 447: > 445: loader = getLoader(url); > 446: // If the loader defines a local class path then > construct > 447: // and add the URLs as the next URLs to be opened. The change to URLClassLoad looks okay although I think the original comment was a bit clearer, no need to insert "then construct" here as this is just getting the class path URLs. src/java.base/share/classes/jdk/internal/loader/URLClassPath.java line 453: > 451: if (DEBUG) { > 452: System.err.println("Failed to construct a loader or > construct its" + > 453: " local classpath for " + url + ", cause:" + > e); At some point we should probably get rid of the DEBUG stuff, it looks like it was left over from early JDK releases. src/java.base/share/classes/jdk/internal/loader/URLClassPath.java line 482: > 480: > 481: // closes the given loader and ignores any IOException that may > occur during close > 482: private static void closeLoaderIgnoreEx(final Loader loader) { closeQuietly(Loader loader) would work too. test/jdk/java/net/URLClassLoader/JarLoaderCloseTest.java line 78: > 76: > 77: /* > 78: * Creates a jar with a manifest which has a Class-Path entry value > with malformed URLs. I think you mean a "a JAR file" rather than "jar" here. Same nit in testParsableClassPathEntry. ------------- PR Review: https://git.openjdk.org/jdk/pull/20691#pullrequestreview-2259319752 PR Review Comment: https://git.openjdk.org/jdk/pull/20691#discussion_r1730378354 PR Review Comment: https://git.openjdk.org/jdk/pull/20691#discussion_r1730378430 PR Review Comment: https://git.openjdk.org/jdk/pull/20691#discussion_r1730378528 PR Review Comment: https://git.openjdk.org/jdk/pull/20691#discussion_r1730379053
