On Mon, 26 Aug 2024 03:47: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. > > Jaikiran Pai has updated the pull request incrementally with three additional > commits since the last revision: > > - revert to old code comment > - use "JAR file" consistently in test's code comments > - rename closeLoaderIgnoreEx to closeQuietly Simplify test data provision by replacing `MethodSource` with `ValueSource`. > `@ValueSource` is one of the simplest possible sources. It lets you specify a > single array of literal values and can only be used for providing a single > argument per parameterized test invocation. Find more details about "[Sources of Arguments](https://junit.org/junit5/docs/current/user-guide/#writing-tests-parameterized-tests-sources)" in JUnit's User-Guide. test/jdk/java/net/URLClassLoader/JarLoaderCloseTest.java line 38: > 36: import org.junit.jupiter.params.ParameterizedTest; > 37: import org.junit.jupiter.params.provider.Arguments; > 38: import org.junit.jupiter.params.provider.MethodSource; Suggestion: import org.junit.jupiter.params.provider.ValueSource; test/jdk/java/net/URLClassLoader/JarLoaderCloseTest.java line 75: > 73: Arguments.of("lib4.jar C:\\bar\\foo\\world/hello.jar") > 74: ); > 75: } Suggestion: test/jdk/java/net/URLClassLoader/JarLoaderCloseTest.java line 84: > 82: */ > 83: @ParameterizedTest > 84: @MethodSource("malformedClassPaths") Suggestion: @ValueSource(strings={ "C:\\foo\\bar\\hello/world.jar lib2.jar", "C:/hello/world/foo.jar", "lib4.jar C:\\bar\\foo\\world/hello.jar" }) test/jdk/java/net/URLClassLoader/JarLoaderCloseTest.java line 114: > 112: Arguments.of("lib10.jar") > 113: ); > 114: } Suggestion: test/jdk/java/net/URLClassLoader/JarLoaderCloseTest.java line 124: > 122: */ > 123: @ParameterizedTest > 124: @MethodSource("missingButParsableClassPaths") Suggestion: @ValueSource(strings={"/home/me/hello/world.jar lib9.jar", "lib10.jar"}) ------------- Changes requested by cstein (Committer). PR Review: https://git.openjdk.org/jdk/pull/20691#pullrequestreview-2262297032 PR Review Comment: https://git.openjdk.org/jdk/pull/20691#discussion_r1732237021 PR Review Comment: https://git.openjdk.org/jdk/pull/20691#discussion_r1732236791 PR Review Comment: https://git.openjdk.org/jdk/pull/20691#discussion_r1732236240 PR Review Comment: https://git.openjdk.org/jdk/pull/20691#discussion_r1732237567 PR Review Comment: https://git.openjdk.org/jdk/pull/20691#discussion_r1732238441
