On Tue, 29 Jul 2025 17:02:01 GMT, Alan Bateman <[email protected]> wrote:
>> David Beaumont has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Removing direct references to JrtFS.
>
> src/java.base/share/classes/jdk/internal/module/SystemModuleFinders.java line
> 257:
>
>> 255: private static Stream<ModuleInfo.Attributes> allModuleAttributes() {
>> 256: // System reader is a singleton and should not be closed by
>> callers.
>> 257: ImageReader reader = SystemImage.reader();
>
> The changes to SystemModuleFinders looks okay but I think we should drop the
> "System reader is a singleton and should not be closed by callers" here as it
> sets doubt that the ImageReader might be closed.
I mean without that, as a reader of the code who doesn't know details, I'd
definitely be questioning why a closeable object (ImageReader) is being
obtained and used, but not closed. I probably even added it because I'd had to
dig around the code to understand that it was actually correct to not close it
here.
Perhaps the docs for SystemImage (currently `Holder class for the ImageReader.`
could be improved to make the lifecycle clear and benefit all callers of
`reader()` ?)
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/26054#discussion_r2240555715
