RFR: 8344908: URLClassPath should not propagate IllegalArgumentException when finding resources in classpath URLs [v3]

[Eirik Bjørsnøs]

On Mon, 25 Nov 2024 08:24:07 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

>> Can I please get a review of this change that proposes to address the issue noted in https://bugs.openjdk.org/browse/JDK-8344908?
>> 
>> With this change, the URLClassPath will no longer propagate the `IllegalArgumentException` thrown when constructing a resource loader. The URL which caused the issue will continue to not be able to serve resources through that URLClassPath but the URLClassPath itself will now be functionally usable for locating resources through rest of the URLs. 
>> 
>> While at it, the internal class `FileURLMapper` which gets used in this code flow has been updated to be more confined by making it package private.
>> 
>> For addressing the underlying issue with ParseUtil we have https://bugs.openjdk.org/browse/JDK-8258246 which I plan to look into separately. There are also a few other usages of `ParseUtil.decode()` in various other places outside of this code flow which I plan to look into separately.
>> 
>> A new jtreg test has been introduced which reproduces the issue and verifies the fix. This test and other existing tests in tier1, tier2 and tier3 continue to pass with this change.
>
> Jaikiran Pai has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - Alan's review - rename variable and drop final from test
>  - Alan's review - no need for throws in method signature

Left some suggestion and remarks for the test

test/jdk/jdk/internal/loader/URLClassPath/ClassPathUnusableURLs.java line 71:

> 69:             // if we can't create a directory with an emoji in its path name,
> 70:             // then skip the entire test
> 71:             assumeTrue(false, "Skipping test since emoji directory couldn't be created: "

Perhaps `Assumptions.abort(message)` would be more clear here.

test/jdk/jdk/internal/loader/URLClassPath/ClassPathUnusableURLs.java line 78:

> 76: 
> 77:         ASCII_DIR = Files.createTempDirectory(SCRATCH_DIR, "test-urlclasspath");
> 78:         Files.writeString(ASCII_DIR.resolve(RESOURCE_NAME), "hello");

The contents of the resources seems not to matter for the test currently.

Consider using just `Files::createFile` here, or make unique content for each resource such that you can verify the correct content was found in the tests.

test/jdk/jdk/internal/loader/URLClassPath/ClassPathUnusableURLs.java line 174:

> 172:     }
> 173: 
> 174:     private static String[] getClassPathElements() {

This could perhaps be `addClassPathElemements(URLClassPath)` instead for cleaner call sites? 

Or perhaps even `URLClassPath makeClassPath()` since both tests seem to have identical class paths?

test/jdk/jdk/internal/loader/URLClassPath/ClassPathUnusableURLs.java line 180:

> 178:         return new String[]{
> 179:                 // non-existent dir
> 180:                 ASCII_DIR.resolve("non-existent").toString(),

Just an observation: This will not be recognized as a directory, so this will not create a `FileLoader`, but a `JarLoader` (which will throw a `FileNotFoundException`).

For good measure, you could consider adding an actual non-existing directory URL, but I think you'd need to use `URLClassPath::addURL` for that instead of `addFile`.

PS: It's rather weird that a non-exisiting `FileLoader` is added to the class path, while a non-existing JarLoader is ignored. Thorny code indeed.

-------------

PR Review: https://git.openjdk.org/jdk/pull/22351#pullrequestreview-2458508653
PR Review Comment: https://git.openjdk.org/jdk/pull/22351#discussion_r1856659478
PR Review Comment: https://git.openjdk.org/jdk/pull/22351#discussion_r1856875185
PR Review Comment: https://git.openjdk.org/jdk/pull/22351#discussion_r1856844889
PR Review Comment: https://git.openjdk.org/jdk/pull/22351#discussion_r1856728116