8344908: URLClassPath should not propagate IllegalArgumentException when finding resources in classpath URLs #22351

jaikiran · 2024-11-25T00:33:41Z

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.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8344908: URLClassPath should not propagate IllegalArgumentException when finding resources in classpath URLs (Bug - P4)

Reviewers

Alan Bateman (@AlanBateman - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/22351/head:pull/22351
$ git checkout pull/22351

Update a local copy of the PR:
$ git checkout pull/22351
$ git pull https://git.openjdk.org/jdk.git pull/22351/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 22351

View PR using the GUI difftool:
$ git pr show -t 22351

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/22351.diff

Using Webrev

Link to Webrev Comment

…hen finding resources in classpath URLs

bridgekeeper · 2024-11-25T00:34:20Z

👋 Welcome back jpai! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2024-11-25T00:35:00Z

@jaikiran This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8344908: URLClassPath should not propagate IllegalArgumentException when finding resources in classpath URLs

Reviewed-by: alanb

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 94 new commits pushed to the master branch:

ce9d543: 8345119: Some java/foreign tests wrongly assume aligned memory
1a07d54: 8343703: Symbol name cleanups after JEP 479
a0df0a5: 8340731: Cleanup remaining IA64 references in hotspot code
8485cb1: 8344822: CDS BulkLoaderTest.java#dynamic fails with COH
f51363e: 8344913: Improve -Xlog:cds+map+oop logging for Java mirrors
cf5ee0b: 8342280: Deprecate for removal java.awt.AWTPermission
8ad0b2a: 8345001: java/awt/doc-files/FocusSpec.html has SecurityManager references
f6d2990: 8344824: CDS dump crashes when member_method of a lambda proxy is null
15378a7: 8345126: [BACKOUT] JDK-8318127: align_up has potential overflow
880f9a7: 8344220: Remove calls to SecurityManager and doPrivileged in java.net.InetAddress and sun.net.util.IPAddressUtil after JEP 486 integration
... and 84 more: https://git.openjdk.org/jdk/compare/a6220fa90362980fce2fc56e70c7c9a7ed7e11c5...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2024-11-25T00:35:37Z

@jaikiran The following labels will be automatically applied to this pull request:

core-libs
net

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

mlbridge · 2024-11-25T00:41:33Z

Webrevs

AlanBateman · 2024-11-25T08:08:18Z

src/java.base/share/classes/sun/net/www/ParseUtil.java

     */
-    public static String decode(String s) {
+    public static String decode(String s) throws IllegalArgumentException {


I'm not sure if you meant to add this or not but "throws IAE" is not needed, usually don't put unchecked exceptions in the throws.

Hello Alan, that was intentional to be more prominently visible, but I'll remove it and will just retain @throws javadoc tag that I added as part of this change.

Done - I've updated the PR to remove the throws clause from the method signature.

AlanBateman · 2024-11-25T08:08:44Z

src/java.base/unix/classes/jdk/internal/loader/FileURLMapper.java

        if (s == null) {
            return false;
        } else {
-            File f = new File (s);
+            File f = new File(s);


Lots of unrelated cleanup in this file, but okay.

Right, I was considering not doing it, but the number of such lines didn't seem too many, so decided to include it. If you and others lean towards leaving the formatting clean up out of this, then I will update the PR accordingly.

AlanBateman · 2024-11-25T08:11:52Z

test/jdk/jdk/internal/loader/URLClassPath/ClassPathUnusableURLs.java

+            final JarEntry jarEntry = new JarEntry(RESOURCE_NAME);
+            jaros.putNextEntry(jarEntry);
+            jaros.write("hello".getBytes(US_ASCII));
+            jaros.closeEntry();


From a readability point of view, I think renaming "jaros" to "jos" would make it a bit easier to read. Also personally I would drop the uses of final from the try statement, only because there it adds noise when looking at the 3 resources in the statement.

I think renaming "jaros" to "jos" would make it a bit easier to read.

Done.

Also personally I would drop the uses of final from the try statement, only because there it adds noise when looking at the 3 resources in the statement.

Understood. I have updated the PR to remove them from the try statement.

eirbjo

Left some suggestion and remarks for the test

eirbjo · 2024-11-25T13:55:30Z

test/jdk/jdk/internal/loader/URLClassPath/ClassPathUnusableURLs.java

+            iae.printStackTrace(); // for debug purpose
+            // if we can't create a directory with an emoji in its path name,
+            // then skip the entire test
+            assumeTrue(false, "Skipping test since emoji directory couldn't be created: "


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

Hello Eirik, it looks like this is a new API introduced in 5.9 of jupiter API. Looking at jtreg 7.4 that's used in JDK mainline, the jupiter API we use is 5.10.x. So yes, abort() would be more appropriate here. I've updated accordingly.

eirbjo · 2024-11-25T14:36:47Z

test/jdk/jdk/internal/loader/URLClassPath/ClassPathUnusableURLs.java

+        // path that can serve the resource.
+        return new String[]{
+                // non-existent dir
+                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.

The use of the "non-existent" path was intentional to trigger the exceptional code path. But I agree that the code comment which said non-existent dir was inaccurate. So I've updated that comment to say "non-existent path"

eirbjo · 2024-11-25T15:44:50Z

test/jdk/jdk/internal/loader/URLClassPath/ClassPathUnusableURLs.java

+        }
+    }
+
+    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?

I prefer keeping the classpath construction through URLClassPath.addFile() closer to the test code itself. So I've left this part as-is.

eirbjo · 2024-11-25T16:02:47Z

test/jdk/jdk/internal/loader/URLClassPath/ClassPathUnusableURLs.java

+        Files.writeString(EMOJI_DIR.resolve(RESOURCE_NAME), "hello");
+
+        ASCII_DIR = Files.createTempDirectory(SCRATCH_DIR, "test-urlclasspath");
+        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.

Done - I've updated the test to use Files.createFile instead of writeString().

jaikiran · 2024-11-27T01:46:30Z

I'll need a Reviewer's review on this one please - the latest update is trivial changes in the test and the test continues to pass.

jaikiran · 2024-11-28T07:37:01Z

Thank you Alan and Eirik for the reviews and inputs.

jaikiran · 2024-11-28T07:53:03Z

/integrate

openjdk · 2024-11-28T07:54:05Z

Going to push as commit 81c44e5.
Since your change was applied there have been 94 commits pushed to the master branch:

ce9d543: 8345119: Some java/foreign tests wrongly assume aligned memory
1a07d54: 8343703: Symbol name cleanups after JEP 479
a0df0a5: 8340731: Cleanup remaining IA64 references in hotspot code
8485cb1: 8344822: CDS BulkLoaderTest.java#dynamic fails with COH
f51363e: 8344913: Improve -Xlog:cds+map+oop logging for Java mirrors
cf5ee0b: 8342280: Deprecate for removal java.awt.AWTPermission
8ad0b2a: 8345001: java/awt/doc-files/FocusSpec.html has SecurityManager references
f6d2990: 8344824: CDS dump crashes when member_method of a lambda proxy is null
15378a7: 8345126: [BACKOUT] JDK-8318127: align_up has potential overflow
880f9a7: 8344220: Remove calls to SecurityManager and doPrivileged in java.net.InetAddress and sun.net.util.IPAddressUtil after JEP 486 integration
... and 84 more: https://git.openjdk.org/jdk/compare/a6220fa90362980fce2fc56e70c7c9a7ed7e11c5...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-11-28T07:54:15Z

@jaikiran Pushed as commit 81c44e5.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

8344908: URLClassPath should not propagate IllegalArgumentException w…

ea6449b

…hen finding resources in classpath URLs

openjdk bot added the rfr Pull request is ready for review label Nov 25, 2024

openjdk bot added core-libs core-libs-dev@openjdk.org net net-dev@openjdk.org labels Nov 25, 2024

minor code comment update in the test

9f9a40d

AlanBateman reviewed Nov 25, 2024

View reviewed changes

jaikiran added 2 commits November 25, 2024 13:42

Alan's review - no need for throws in method signature

f6c8da5

Alan's review - rename variable and drop final from test

20674bd

AlanBateman approved these changes Nov 25, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Nov 25, 2024

eirbjo reviewed Nov 25, 2024

View reviewed changes

jaikiran added 3 commits November 26, 2024 12:02

use Assumptions.abort() in test

621602c

Files.createFile() instead of Files.writeString()

ff99016

minor code comment change

c348272

openjdk bot removed the ready Pull request is ready to be integrated label Nov 26, 2024

AlanBateman approved these changes Nov 28, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Nov 28, 2024

openjdk bot added the integrated Pull request has been integrated label Nov 28, 2024

openjdk bot closed this Nov 28, 2024

openjdk bot removed the ready Pull request is ready to be integrated label Nov 28, 2024

openjdk bot removed the rfr Pull request is ready for review label Nov 28, 2024

jaikiran deleted the 8344908 branch November 28, 2024 07:54

8344908: URLClassPath should not propagate IllegalArgumentException when finding resources in classpath URLs #22351

8344908: URLClassPath should not propagate IllegalArgumentException when finding resources in classpath URLs #22351

Uh oh!

Conversation

jaikiran commented Nov 25, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Nov 25, 2024

Uh oh!

openjdk bot commented Nov 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Nov 25, 2024

Uh oh!

mlbridge bot commented Nov 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaikiran Nov 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eirbjo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaikiran commented Nov 27, 2024

Uh oh!

jaikiran commented Nov 28, 2024

Uh oh!

jaikiran commented Nov 28, 2024

Uh oh!

openjdk bot commented Nov 28, 2024

Uh oh!

openjdk bot commented Nov 28, 2024

Uh oh!

Uh oh!

jaikiran commented Nov 25, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Nov 25, 2024 •

edited

Loading

mlbridge bot commented Nov 25, 2024 •

edited

Loading

jaikiran Nov 25, 2024 •

edited

Loading