RFR: 8347712: IllegalStateException on multithreaded ZipFile access with non-UTF8 charset [v4]

[Jaikiran Pai]

On Thu, 13 Mar 2025 19:31:31 GMT, Eirik Bjørsnøs <eirbjo at openjdk.org> wrote:

>> Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   add code comment
>
> test/jdk/java/util/zip/ZipFile/ZipFileSharedSourceTest.java line 78:
> 
>> 76: 
>> 77:     /**
>> 78:      * Creates multiple instances of java.util.zip.ZipFile with the given {@code entryNameCharset}
> 
> This comment could be interpreted as saying that the construction of the ZipFile instances happens up-front, on the main test thread. However, looking at the actual code, the construction of the ZipFile instance AND the enumeration happens concurrently.
> 
> Perhaps we could trim the "how" part here, and instad just say that the test verifies that concurrent construction and enumeration of  java.util.zip.ZipFile instances backed by a the same ZIP file does not cause unexpected encoding-related concurrency failures?

I've now updated the test method comment to clarify what the test does.

> test/jdk/java/util/zip/ZipFile/ZipFileSharedSourceTest.java line 86:
> 
>> 84:     @ParameterizedTest
>> 85:     @MethodSource("charsets")
>> 86:     void testMultipleZipFileInstances(final Charset entryNameCommentCharset) throws Exception {
> 
> The Charset parameter could be renamed to `charset`.

Done.

> test/jdk/java/util/zip/ZipFile/ZipFileSharedSourceTest.java line 111:
> 
>> 109:         private final CountDownLatch startLatch;
>> 110: 
>> 111:         private ZipEntryIteratingTask(final Path file, final Charset entryNameCharset,
> 
> The Charset parameter/field could be renamed to `charset`.

Done.

> test/jdk/java/util/zip/ZipFile/ZipFileSharedSourceTest.java line 127:
> 
>> 125:             try (final ZipFile zf = new ZipFile(this.file.toFile(), this.entryNameCharset)) {
>> 126:                 final var entries = zf.entries();
>> 127:                 while (entries.hasMoreElements()) {
> 
> I think it would be good to include a call to `ZipFile::getEntry` here as well, since that exercises a separate code path.

Done. The updated PR now does a `ZipFile.getEntry()` call too.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/23986#discussion_r2003489767
PR Review Comment: https://git.openjdk.org/jdk/pull/23986#discussion_r2003491140
PR Review Comment: https://git.openjdk.org/jdk/pull/23986#discussion_r2003490799
PR Review Comment: https://git.openjdk.org/jdk/pull/23986#discussion_r2003490505