On Tue, 9 Jun 2026 11:11:01 GMT, Dusan Balek <[email protected]> wrote:
>> When used with the `--system` option to target a different modularized JDK, >> `JavacTask` keeps both `lib/jrt-fs.jar` and `lib/modules` open even after it >> completes. >> >> The `lib/jrt-fs.jar` file remains open because the host-side >> `JrtFileSystemProvider` creates a `URLClassLoader` to load the target >> platform's `JrtFileSystemProvider` from `lib/jrt-fs.jar` when creating a >> `JrtFileSystem`. This `URLClassLoader` is never closed, causing the file to >> remain open. >> >> The `lib/modules` file remains open because `BasicImageReader` uses a >> memory-mapped file whose mapping persists until the associated buffer is >> garbage-collected. >> >> To ensure that `lib/jrt-fs.jar` is properly closed, the proposed solution is >> to make the client responsible for closing the file system's class loader >> when "java.home" is passed in the `env` argument to >> `FileSystems.newFileSystem(...)`. >> This approach (currently implemented in the `Locations` class) is simple and >> works reliably even with older target platform versions. (An alternative >> approach would be to automatically close the `URLClassLoader` when the >> `JrtFileSystem` is closed. However, this solution has two significant >> drawbacks. First, it does not work with older target platform versions >> because it requires modifications to the target platform's `JrtFileSystem` >> implementation. Second, it is not reliable. A user may obtain a reference to >> the provider that created a `JrtFileSystem` instance, close that instance, >> and then use the same provider to create another `JrtFileSystem`. In that >> case, the provider's `URLClassLoader` may have already been closed >> automatically, preventing the creation of the new file system instance). >> >> To ensure that `lib/modules` is properly closed, the proposed solution is to >> modify `BasicImageReader` so that it uses memory-mapped I/O only for the >> current runtime and falls back to regular file I/O when accessing a >> different target system. (An alternative solution would be to explicitly >> unmap the memory-mapped file when the reader is closed. On JDK 22 and later, >> the Foreign Function & Memory API could be used for that purpose. However, >> in that case, `lib/jrt-fs.jar` could no longer be compiled to run on JDK 8). >> Note that neither of these solutions work for older target platform >> versions, as both require changes to the target platform's >> `BasicImageReader` implementation. >> >> >> --------- >> - [x] I confirm that I make this contribution in accordance with the >> [OpenJDK Interim AI Policy](https://openjdk.or... > > Dusan Balek has updated the pull request incrementally with one additional > commit since the last revision: > > Real path used as filter Updating implementation looks good. Having the test use jlink to create a run-time image is good, just a few questions on the test as the handling of compilation failures or other errors isn't clear. src/java.base/share/classes/jdk/internal/jimage/BasicImageReader.java line 96: > 94: this.name = this.imagePath.toString(); > 95: > 96: // Only the current runtime image is loaded with the root > class-loader. The comment is still a bit confusing, I think you want "The image reader will be for the current run-time image when the this class is defined by the boot class loader". test/langtools/tools/javac/SystemFilesClosed.java line 29: > 27: * @summary Check that `lib/jrt-fs.jar` and `lib/modules` are properly > closed while > 28: * javac is invoked with `--system` option. > 29: * @requires os.family == "mac" | os.family == "linux" Is the losf usage is removed, could this test run on Windows? test/langtools/tools/javac/SystemFilesClosed.java line 59: > 57: void testSystemFilesClosed() throws Exception { > 58: String targetSystem = base.toString(); > 59: try (PrintStream err = new > PrintStream(OutputStream.nullOutputStream())) { Why is stderr going to the equivalent of /dev/null? I would think it would be better to invoke jlink with the current System.out and System.err so that the output/error goes to the .jtr file. test/langtools/tools/javac/SystemFilesClosed.java line 84: > 82: > 83: try (StandardJavaFileManager fileManager = > compiler.getStandardFileManager(null, null, null)) { > 84: compiler.getTask(null, fileManager, null, List.of("--system", > targetSystem), null, List.of(compilationUnit)) I'm puzzled by the need to fork lsof to look at the output of the current process. I'm wondering if the return from call should be checked so that the test fails if it returns false. ------------- PR Review: https://git.openjdk.org/jdk/pull/31417#pullrequestreview-4458064277 PR Review Comment: https://git.openjdk.org/jdk/pull/31417#discussion_r3380299737 PR Review Comment: https://git.openjdk.org/jdk/pull/31417#discussion_r3380183899 PR Review Comment: https://git.openjdk.org/jdk/pull/31417#discussion_r3380195773 PR Review Comment: https://git.openjdk.org/jdk/pull/31417#discussion_r3380254889
