RFR: 8372787: ModuleReader should throw IOException consistently when using --patch-module and ModuleReader is closed [v3]

[Alan Bateman]

On Mon, 1 Dec 2025 10:55:59 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

>> Can I please get a review of this change which proposes to address the issue noted in https://bugs.openjdk.org/browse/JDK-8372787?
>> 
>> The commit in this PR takes into account the `IllegalStateException` thrown by `JarFile` APIs and wraps them into a `IOException` to conform with the expectations of the `ModuleReader` APIs.
>> 
>> A new jtreg test has been introduced to reproduce the issue and verify the fix. CI testing is currently in progress with this change.
>
> Jaikiran Pai has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - move ensureOpen() to a few lines up
>  - assert instead of ensureOpen()

src/java.base/share/classes/jdk/internal/module/ModulePatcher.java line 295:

> 293:         }
> 294: 
> 295:         private void ensureOpen() throws IOException {

The private methods have a short description so you'll want to do the same here.

test/jdk/java/lang/module/ModuleReader/patched/PatchedModuleReaderTest.java line 44:

> 42:  * @run junit/othervm ${test.main.class}
> 43:  */
> 44: class PatchedModuleReaderTest {

The suggests that it's a complete test for a ModuleReader when the module is patched but it is more specific so I think my main main is to rename and split up testIOExceptionUponClose so that it is a more complete test for a ModuleReader when the module has been patched with --patch-module.

test/jdk/java/lang/module/ModuleReader/patched/PatchedModuleReaderTest.java line 54:

> 52:      */
> 53:     @Test
> 54:     void testIOExceptionUponClose() throws Exception {

"AfterClose" rather than "UponClose".

test/jdk/java/lang/module/ModuleReader/patched/PatchedModuleReaderTest.java line 59:

> 57:                 .orElseThrow();
> 58:         final ModuleReader reader;
> 59:         final Stream<String> resources;

The use of finals on the locals to ensure a single assignment is a distraction here. Once this test method is split once into small tests then you can get rid of the finals.

test/jdk/java/lang/module/ModuleReader/patched/PatchedModuleReaderTest.java line 64:

> 62:         try (var _ = reader = mref.open()) {
> 63:             // verify we are using the patched module
> 64:             final String resourceName = "java/lang/PatchedFoo.class";

This should be a separate test method.

test/jdk/java/lang/module/ModuleReader/patched/PatchedModuleReaderTest.java line 71:

> 69:             assertTrue(reader.find(nonExistentResource).isEmpty(),
> 70:                     "unexpected resource " + nonExistentResource + " in "
> 71:                             + mref.descriptor().name() + " module");

This should be a separate test method.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/28569#discussion_r2576595215
PR Review Comment: https://git.openjdk.org/jdk/pull/28569#discussion_r2576621876
PR Review Comment: https://git.openjdk.org/jdk/pull/28569#discussion_r2576600609
PR Review Comment: https://git.openjdk.org/jdk/pull/28569#discussion_r2576619469
PR Review Comment: https://git.openjdk.org/jdk/pull/28569#discussion_r2576615378
PR Review Comment: https://git.openjdk.org/jdk/pull/28569#discussion_r2576615962