On Mon, 25 Nov 2024 08:24:07 GMT, Jaikiran Pai <j...@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
