RFR: 8365467: Fix multiple issues in ExplodedImage

Wed Aug 13 12:40:17 UTC 2025

On Wed, 13 Aug 2025 12:20:23 GMT, David Beaumont <duke at openjdk.org> wrote:

> 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.

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

src/java.base/share/classes/jdk/internal/jrtfs/ExplodedImage.java line 59:

> 57:     private static final String PACKAGES = "/packages/";
> 58: 
> 59:     private final Path modulesDir;

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

src/java.base/share/classes/jdk/internal/jrtfs/ExplodedImage.java line 65:

> 63: 
> 64:     ExplodedImage(Path modulesDir) throws IOException {
> 65:         this.modulesDir = modulesDir;

Avoid unnecessary assumptions about file systems.

src/java.base/share/classes/jdk/internal/jrtfs/ExplodedImage.java line 95:

> 93:         }
> 94: 
> 95:         @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.

src/java.base/share/classes/jdk/internal/jrtfs/ExplodedImage.java line 132:

> 130:                 try (DirectoryStream<Path> stream = Files.newDirectoryStream(path)) {
> 131:                     for (Path p : stream) {
> 132:                         p = modulesDir.relativize(p);

Avoid using static field.

src/java.base/share/classes/jdk/internal/jrtfs/ExplodedImage.java line 158:

> 156: 
> 157:     @Override
> 158:     public synchronized void close() throws IOException {

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

src/java.base/share/classes/jdk/internal/jrtfs/ExplodedImage.java line 171:

> 169:             return node;
> 170:         }
> 171:         // lazily created for paths like /packages/<package>/<module>/xyz

This code is simply wrong. The lookup of 'packages/java.lang/java.base/java/lang' should fail, since there is no node for it. It's the wrapping file system's job to test for symbolic links in the path and resolve this sort of thing.
Only /packages, /packages/xxx and /packages/xxx/yyy nodes need to exist in SystemImage.

src/java.base/share/classes/jdk/internal/jrtfs/ExplodedImage.java line 216:

> 214:         } catch (IOException x) {
> 215:             // Since the path reference a file, any errors should not be ignored.
> 216:             throw new UncheckedIOException(x);

Silently ignoring IOException was both a risk, and a possible performance issue, since it was being used for normal code flow whenever a non-existent node was being asked for. Now, an exception here is unconditionally a problem, since the given path does exist.

src/java.base/share/classes/jdk/internal/jrtfs/ExplodedImage.java line 226:

> 224:      */
> 225:     Path underlyingModulesPath(String name) {
> 226:         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.

src/java.base/share/classes/jdk/internal/jrtfs/ExplodedImage.java line 258:

> 256:                     String moduleName = module.getFileName().toString();
> 257:                     // make sure "/modules/<moduleName>" is created
> 258:                     Objects.requireNonNull(createModulesNode(MODULES + moduleName, module));

This happens once per module, so it's where we create the root dirs (see below).

src/java.base/share/classes/jdk/internal/jrtfs/ExplodedImage.java line 276:

> 274:         // create "/modules" directory
> 275:         // "nodes" map contains only /modules/<foo> nodes only so far and so add all as children of /modules
> 276:         PathNode modulesRootNode = new PathNode("/modules", new ArrayList<>(nodes.values()));

Renamed because there's already "modulesDir" elsewhere.

src/java.base/share/classes/jdk/internal/jrtfs/ExplodedImage.java line 286:

> 284:             List<Node> moduleLinkNodes = new ArrayList<>(moduleNameList.size());
> 285:             for (String moduleName : moduleNameList) {
> 286:                 Node moduleNode = Objects.requireNonNull(nodes.get(MODULES + moduleName));

There's no need to call code that "creates" the module directory node here, we did it above, and here we can just guarantee it's in the cache.

src/java.base/share/classes/jdk/internal/jrtfs/SystemImage.java line 81:

> 79:     }
> 80: 
> 81:     private static final String RUNTIME_HOME;

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

test/jdk/jdk/internal/jrtfs/whitebox/ExplodedImageTestDriver.java line 30:

> 28:  * @run junit/othervm java.base/jdk.internal.jrtfs.ExplodedImageTest
> 29:  */
> 30: 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.

test/jdk/jdk/internal/jrtfs/whitebox/TEST.properties line 1:

> 1: modules = \

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

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

PR Review: https://git.openjdk.org/jdk/pull/26757#pullrequestreview-3115737360
PR Review Comment: https://git.openjdk.org/jdk/pull/26757#discussion_r2273278111
PR Review Comment: https://git.openjdk.org/jdk/pull/26757#discussion_r2273280133
PR Review Comment: https://git.openjdk.org/jdk/pull/26757#discussion_r2273285948
PR Review Comment: https://git.openjdk.org/jdk/pull/26757#discussion_r2273289787
PR Review Comment: https://git.openjdk.org/jdk/pull/26757#discussion_r2273293717
PR Review Comment: https://git.openjdk.org/jdk/pull/26757#discussion_r2273300351
PR Review Comment: https://git.openjdk.org/jdk/pull/26757#discussion_r2273308453
PR Review Comment: https://git.openjdk.org/jdk/pull/26757#discussion_r2273325040
PR Review Comment: https://git.openjdk.org/jdk/pull/26757#discussion_r2273310217
PR Review Comment: https://git.openjdk.org/jdk/pull/26757#discussion_r2273313715
PR Review Comment: https://git.openjdk.org/jdk/pull/26757#discussion_r2273312772
PR Review Comment: https://git.openjdk.org/jdk/pull/26757#discussion_r2273315520
PR Review Comment: https://git.openjdk.org/jdk/pull/26757#discussion_r2273317906
PR Review Comment: https://git.openjdk.org/jdk/pull/26757#discussion_r2273319482