On Wed, 13 Aug 2025 12:20:23 GMT, David Beaumont <d...@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
