On Tue, 29 Jul 2025 17:52:27 GMT, David Beaumont <d...@openjdk.org> wrote:
>> 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()` ?)
Maybe, or the singleton returned by SystemImage.reader() is non-closeable. My
only comment for now is tha the "should not be called by callers." is confusing
here and would be better to remove. We do do this in the next change in the
area if you want.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/26054#discussion_r2267081379
