On Mon, 1 Dec 2025 10:55:59 GMT, Jaikiran Pai <[email protected]> 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
