8365467: Issues with jrtfs implementation for exploded run-time images #26757

david-beaumont · 2025-08-13T12:20:23Z

This PR addresses several issues found while adding unit tests for ExplodedImage.
I have added review comments for changes (some of which are a little preferential, but bring the code into line with things like ImageReader in terms of the name choices for variables).
Later it is likely that this code will be adapted for the up-coming preview mode support in Valhalla, so having it unit-testable is important.

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-8365467: Issues with jrtfs implementation for exploded run-time images (Bug - P4)

Reviewers

Roger Riggs (@RogerRiggs - Reviewer)
Athijegannathan Sundararajan (@sundararajana - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 26757

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

Using diff file

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

Using Webrev

Link to Webrev Comment

* tweaking tests for readability * Address issues and add unit tests

bridgekeeper · 2025-08-13T12:21:24Z

👋 Welcome back david-beaumont! 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 · 2025-08-13T12:21:35Z

@david-beaumont 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:

8365467: Issues with jrtfs implementation for exploded run-time images

Reviewed-by: rriggs, sundar

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 259 new commits pushed to the master branch:

ecf05ca: 8366693: Backout recent JavaLangAccess changes breaking the build
444a8fa: 8365501: Remove special AdapterHandlerEntry for abstract methods
a029245: 8365983: Tests should throw SkippedException when SCTP not supported
... and 256 more: https://git.openjdk.org/jdk/compare/f3b34d32d6ea409f8c8f0382e8f01e746366f842...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@RogerRiggs, @sundararajana, @liach) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

openjdk · 2025-08-13T12:22:48Z

@david-beaumont The following label will be automatically applied to this pull request:

core-libs

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

mlbridge · 2025-08-13T12:26:54Z

Webrevs

01: Full - Incremental (51a46c8f)
00: Full (1408def0)

david-beaumont

Adding explanatory notes for the non-test changes/fixes.

david-beaumont · 2025-08-13T12:21:42Z

src/java.base/share/classes/jdk/internal/jrtfs/ExplodedImage.java

-    private static final int PACKAGES_LEN = PACKAGES.length();

-    private final FileSystem defaultFS;
+    private final Path modulesDir;


Needed to make this class actually testable and avoid just using the static field from SystemImage.

david-beaumont · 2025-08-13T12:22:11Z

src/java.base/share/classes/jdk/internal/jrtfs/ExplodedImage.java

    ExplodedImage(Path modulesDir) throws IOException {
-        defaultFS = FileSystems.getDefault();
-        String str = defaultFS.getSeparator();
+        this.modulesDir = modulesDir;


Avoid unnecessary assumptions about file systems.

david-beaumont · 2025-08-13T12:23:33Z

src/java.base/share/classes/jdk/internal/jrtfs/ExplodedImage.java

            this.children = children;
        }

+        @Override


Discovered this method was missing during testing. I think this logic is what's needed here, but I would like someone to just double check.

Is that thumbs up and confirmation that you've checked it?

@david-beaumont I think you need to override getLocation if you override isResource so image reader can actually fetch the data - unfortunately this is not clearly documented.

Ooooh. That's interesting.
So before, these nodes were none of the types resource, directory or link.
And obviously that was working to an extent.
And you're right that "isResource()" should imply "getResource()" works, but if that's the case, how can a JRT file system be built on top of this at all?
I need to investigate more...

Ah no, there is not, and never was a "getResource()" method on the node class. Nodes are not inner classes and do not have access to the image.
The getResource() method exists (both now and before my changes) on the ExplocedImage class.
So I think what I've done here is correct, and no other changes are necessary.

I think this was the method I saw:

jdk/src/java.base/share/classes/jdk/internal/jimage/ImageReader.java

Line 572 in 1408def

ImageLocation getLocation() {

It is package private so we cannot override it here. What is it for at all then...

It's for getting the location from a Resource node inside the getResource(Node) method.
ExplodedImage nodes just don't use ImageLocation at all, so cannot need it.
getResource(Node) is overloaded on ExplodedImage and the parent method never called.

david-beaumont · 2025-08-13T12:24:30Z

src/java.base/share/classes/jdk/internal/jrtfs/ExplodedImage.java

                try (DirectoryStream<Path> stream = Files.newDirectoryStream(path)) {
                    for (Path p : stream) {
-                        p = explodedModulesDir.relativize(p);
+                        p = modulesDir.relativize(p);


Avoid using static field.

david-beaumont · 2025-08-13T12:25:29Z

src/java.base/share/classes/jdk/internal/jrtfs/ExplodedImage.java


    @Override
-    public void close() throws IOException {
+    public synchronized void close() throws IOException {


The nodes map is always accessed synchronised except here, so by synchronizing this we can stop using ConcurrentHashMap.

david-beaumont · 2025-08-13T12:32:18Z

src/java.base/share/classes/jdk/internal/jrtfs/ExplodedImage.java

        // "nodes" map contains only /modules/<foo> nodes only so far and so add all as children of /modules
-        PathNode modulesDir = new PathNode("/modules", new ArrayList<>(nodes.values()));
-        nodes.put(modulesDir.getName(), modulesDir);
+        PathNode modulesRootNode = new PathNode("/modules", new ArrayList<>(nodes.values()));


Renamed because there's already "modulesDir" elsewhere.

david-beaumont · 2025-08-13T12:32:58Z

src/java.base/share/classes/jdk/internal/jrtfs/SystemImage.java

    }

-    static final String RUNTIME_HOME;
+    private static final String RUNTIME_HOME;


Hiding these prevents unwanted use by other classes (which would make them effectively untestable).

You can still use Method handles and @modules java.base/jdk.internal.jrtfs:+open in JTReg to test these.

Sure, but anyone who accesses these via method handles deserves all they get (imo).
If these do need to be tested, they can be opened up again. Making them private is taking the temptation to just use them casually away from whoever edits these files next.

david-beaumont · 2025-08-13T12:33:55Z

test/jdk/jdk/internal/jrtfs/whitebox/ExplodedImageTestDriver.java

+ * @modules java.base/jdk.internal.jrtfs java.base/jdk.internal.jimage
+ * @run junit/othervm java.base/jdk.internal.jrtfs.ExplodedImageTest
+ */
+public class ExplodedImageTestDriver {}


Not 100% sure if a class declaration is needed here. It seems to work fine, but I've seen a case where there wasn't one (but that felt odd). Happy to remove if not wanted.

david-beaumont · 2025-08-13T12:34:32Z

test/jdk/jdk/internal/jrtfs/whitebox/TEST.properties

@@ -0,0 +1,4 @@
+modules = \


Cargo-culted without a true understand of what's going on. This seems to be what's needed for module access.

https://openjdk.org/jtreg/tag-spec.html#:~:text=@modules%20%5B/,when%20the%20test%20is%20run

modules <module[/package[:flag]]>+
Express a default dependence, for tests in this directory and its subdirectories, on modules in the system being tested, and optionally, on selected internal packages in some or all of those modules.

david-beaumont · 2025-08-13T12:36:40Z

src/java.base/share/classes/jdk/internal/jrtfs/ExplodedImage.java

+     * path does not reference a file.
+     */
+    Path underlyingModulesPath(String name) {
+        if (name.startsWith(MODULES) && name.length() > MODULES.length()) {


The extra length test avoids issues when "/modules/" is given, since that should be invalid but otherwise gets turned into a path to the parent dir.

RogerRiggs

Looks pretty good.

RogerRiggs · 2025-08-21T19:05:44Z

src/java.base/share/classes/jdk/internal/jrtfs/ExplodedImage.java

+     * @return the newly created and cached node, or {@code null} if the given
+     *     path references a file which must be hidden in the node hierarchy.
+     */
+    Node createModulesNode(String name, Path path) {


This and other methods could be private if not used outside the class.

Done.

There's little consistency in a lot of the access modifiers in the code around here, so I'm defaulting to "don't touch" since I don't know what is, or is not the accepted style. There are lots of other methods here which could/should be private, and it's probably better to raise a PR to go over the entire package and do a sweep for consistency rather than picking these off one-at-a-time.

RogerRiggs · 2025-08-21T19:17:01Z

src/java.base/share/classes/jdk/internal/jrtfs/ExplodedImage.java

+     */
+    Node createModulesNode(String name, Path path) {
+        assert !nodes.containsKey(name) : "Node must not already exist: " + name;
+        assert name.startsWith(MODULES) && name.length() > MODULES.length() : "Invalid modules name: " + name;


startsWith(MODUELES) && name.length(...) might be worth a utility method.
Only 2 uses though.

AlanBateman · 2025-08-29T09:43:40Z

Just some context here. jrtfs has support for exploded images so that it can be used (by javac) in the JDK build. This code is not used once the JDK images are built. It's very likely that some of this code is never executed as javac only uses a subset of the file system API. So good to have tests for this as it would other be untested/unused and bugs will hide until the day that javac expands its usage and hits them.

sundararajana

LGTM

sundararajana · 2025-08-29T10:06:33Z

test/jdk/jdk/internal/jrtfs/whitebox/TEST.properties

@@ -0,0 +1,4 @@
+modules = \


https://openjdk.org/jtreg/tag-spec.html#:~:text=@modules%20%5B/,when%20the%20test%20is%20run

modules <module[/package[:flag]]>+
Express a default dependence, for tests in this directory and its subdirectories, on modules in the system being tested, and optionally, on selected internal packages in some or all of those modules.

david-beaumont · 2025-08-29T11:12:02Z

/integrate

openjdk · 2025-08-29T11:13:07Z

@david-beaumont
Your change (at version 51a46c8) is now ready to be sponsored by a Committer.

liach

Requesting changes for the isResource update.

liach · 2025-08-30T14:22:11Z

src/java.base/share/classes/jdk/internal/jrtfs/ExplodedImage.java

            this.children = children;
        }

+        @Override


@david-beaumont I think you need to override getLocation if you override isResource so image reader can actually fetch the data - unfortunately this is not clearly documented.

liach · 2025-08-30T14:23:33Z

src/java.base/share/classes/jdk/internal/jrtfs/SystemImage.java

    }

-    static final String RUNTIME_HOME;
+    private static final String RUNTIME_HOME;


You can still use Method handles and @modules java.base/jdk.internal.jrtfs:+open in JTReg to test these.

RogerRiggs

Looks good.

david-beaumont · 2025-09-04T09:12:48Z

/integrate

openjdk · 2025-09-04T09:13:44Z

@david-beaumont
Your change (at version 51a46c8) is now ready to be sponsored by a Committer.

RogerRiggs · 2025-09-04T13:17:21Z

/sponsor
Thanks for the refactoring and cleanup.

openjdk · 2025-09-04T13:19:18Z

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

80873a0: 8366836: Don't execute post-IncludeCustomExtension if file was not included
8c50bed: 8366872: Wrong number of template arguments in test in test_rbtree.cpp
53d4e92: 8366238: Improve RBTree API with stricter comparator semantics and pluggable validation/printing hooks
... and 293 more: https://git.openjdk.org/jdk/compare/f3b34d32d6ea409f8c8f0382e8f01e746366f842...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-09-04T13:19:29Z

@RogerRiggs @david-beaumont Pushed as commit e190355.

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

Provisional fixes for ExplodedImage issues.

1408def

* tweaking tests for readability * Address issues and add unit tests

openjdk bot added core-libs core-libs-dev@openjdk.org rfr Pull request is ready for review labels Aug 13, 2025

david-beaumont commented Aug 13, 2025

View reviewed changes

RogerRiggs reviewed Aug 21, 2025

View reviewed changes

Feedback tweaks.

51a46c8

sundararajana approved these changes Aug 29, 2025

View reviewed changes

david-beaumont changed the title ~~8365467: Fix multiple issues in ExplodedImage~~ 8365467: Issues with jrtfs implementation for exploded run-time images Aug 29, 2025

openjdk bot added the ready Pull request is ready to be integrated label Aug 29, 2025

openjdk bot added the sponsor Pull request is ready to be sponsored label Aug 29, 2025

liach suggested changes Aug 30, 2025

View reviewed changes

RogerRiggs approved these changes Sep 2, 2025

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Sep 4, 2025

openjdk bot closed this Sep 4, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Sep 4, 2025

8365467: Issues with jrtfs implementation for exploded run-time images #26757

8365467: Issues with jrtfs implementation for exploded run-time images #26757

Uh oh!

Conversation

david-beaumont commented Aug 13, 2025 • 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 Aug 13, 2025

Uh oh!

openjdk bot commented Aug 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Aug 13, 2025

Uh oh!

mlbridge bot commented Aug 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

david-beaumont 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!

david-beaumont Sep 4, 2025 • 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!

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!

RogerRiggs 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!

AlanBateman commented Aug 29, 2025

Uh oh!

sundararajana left a comment

Choose a reason for hiding this comment

david-beaumont commented Aug 13, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Aug 13, 2025 •

edited

Loading

mlbridge bot commented Aug 13, 2025 •

edited

Loading

david-beaumont Sep 4, 2025 •

edited

Loading