On Tue, 27 Aug 2024 08:38:23 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. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > Christian's suggestion - string ValueSource(s) instead of MethodSource Update looks reasonable ------------- Marked as reviewed by michaelm (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20691#pullrequestreview-2265658939
