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

[Jaikiran Pai]

On Sat, 22 Mar 2025 17:21:49 GMT, Alan Bateman <alanb at openjdk.org> wrote:

>> Jaikiran Pai has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains nine additional commits since the last revision:
>> 
>>  - Eirik's suggestion - update test method comment
>>  - rename entryNameCharset to charset in the test
>>  - Eirik's suggestion - update test to even call ZipFile.getEntry()
>>  - Eirik's inputs - replace useUTF8Coder() with zipCoderFor()
>>  - merge latest from master branch
>>  - add code comment
>>  - rename field
>>  - tiny typo fix in newly introduced documentation
>>  - 8347712: IllegalStateException on multithreaded ZipFile access with non-UTF8 charset
>
> src/java.base/share/classes/java/util/zip/ZipCoder.java line 47:
> 
>> 45:  * and can be used concurrently by multiple threads. All other {@code ZipCoder} instances
>> 46:  * are not thread-safe and external synchronization is required by callers, if those
>> 47:  * instances are to be used concurrently by multiple threads.
> 
> I think you can trim this down to just say that the UTF-8 ZipCoder is thread safe, ZipCoder for other charsets require synchronization.

Done - the PR has been updated to trim this text to what you suggested.

> src/java.base/share/classes/java/util/zip/ZipFile.java line 1146:
> 
>> 1144:     static record EntryPos(String name, int pos) {}
>> 1145: 
>> 1146:     // Implementation note: This class must be thread-safe.
> 
> I think you mean "This class is thread safe".

I was trying to phrase it as text that provides guidance to future maintainers of this class. That's why the use of "must" and the additional text explaining why the "must". But your proposed simpler version is fine too. I have updated the PR to use this text and removed the latter part of the comment.

> src/java.base/share/classes/java/util/zip/ZipFile.java line 1149:
> 
>> 1147:     // Each instance of Source could be shared between multiple different instances of ZipFile.
>> 1148:     // Although ZipFile isn't thread-safe, the fact that separate instances of ZipFile could
>> 1149:     // still concurrently use the same Source instance, mandates that Source must be thread-safe.
> 
> If you assert previously that the class is thread safe then I think you can drop this confusing comment.

Done.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/23986#discussion_r2009057907
PR Review Comment: https://git.openjdk.org/jdk/pull/23986#discussion_r2009058350
PR Review Comment: https://git.openjdk.org/jdk/pull/23986#discussion_r2009058375