On Mon, 1 Dec 2025 18:18:07 GMT, Alan Bateman <[email protected]> wrote:
>> Jaikiran Pai has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Alan's suggestion - additional tests to the found resource URI
>
> src/java.base/share/classes/jdk/internal/module/ModulePatcher.java line 424:
>
>> 422: @Override
>> 423: public void close() throws IOException {
>> 424: closed = true;
>
> It might be a bit better to avoid the closeAll+delegate if already closed.
Hello Alan, I've now updated the PR to return early if already closed. I have
kept it a simple check and it won't take into account concurrent calls to
`close()`. I think that's OK given what we discussed in this PR about async
close. However, if you would like to have this be a bit more stronger (like we
do with the `delegate` creation), then let me know and I will adjust this check
in `close()` accordingly.
> test/jdk/java/lang/module/ModuleReader/patched/PatchedModuleReaderTest.java
> line 40:
>
>> 38: /*
>> 39: * @test
>> 40: * @bug 8372787
>
> Can you fix the `@summary` so that it starts with "Test the .." or "Verify
> the ..."
Done.
> test/jdk/java/lang/module/ModuleReader/patched/PatchedModuleReaderTest.java
> line 110:
>
>> 108:
>> 109: // verify IOException is thrown by the closed ModuleReader
>> 110: resources.forEach(rn -> {
>
> The test should also check reader.list() throws IOException.
Good catch. It was there previously but I lost that check after I refactored
the first version of this test. I've added it back now.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/28569#discussion_r2579872453
PR Review Comment: https://git.openjdk.org/jdk/pull/28569#discussion_r2579873014
PR Review Comment: https://git.openjdk.org/jdk/pull/28569#discussion_r2579879350
