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

[Jaikiran Pai]

On Mon, 24 Mar 2025 12:52:49 GMT, Eirik Bjørsnøs <eirbjo at openjdk.org> wrote:

>> Jaikiran Pai has updated the pull request incrementally with three additional commits since the last revision:
>> 
>>  - improve code comment for ZipFile.zipCoder
>>  - Alan's suggestion - change code comment about Source class being thread safe
>>  - Alan's suggestion - trim the javadoc of (internal) ZipCoder class
>
> src/java.base/share/classes/java/util/zip/ZipCoder.java line 181:
> 
>> 179: 
>> 180:     /**
>> 181:      * {@return the {@link Charset} used this {@code ZipCoder}}
> 
> Suggestion:
> 
>      * {@return the {@link Charset} used by this {@code ZipCoder}}

Fixed.

> src/java.base/share/classes/java/util/zip/ZipFile.java line 85:
> 
>> 83:     private final String filePath;     // ZIP file path
>> 84:     private final String fileName;     // name of the file
>> 85:     // ZipCoder for entry names and comments when not using UTF-8
> 
> "_when not using  UTF-8_"  could be confusing.
> 
> The ZipCoder here may well be UTF-8, it's more about the entry not mandating UTF-8 by having its language encoding flag set.
> 
> I think we should either clearly explain when this ZipCoder is used (when entries do not mandate UTF-8), or drop the information here and lean on it being explained in `zipCoderFor`.
> 
> If we decide to drop it, this could be just:
> 
> `// Used when decoding entry names and comments`
> 
> If we decide to keep it, then something like:
> 
> `// Used when decoding entry names and comments, unless entry flags mandate UTF-8`

I used your suggestion of "Used when decoding entry names and comments" to keep it simple.

> src/java.base/share/classes/java/util/zip/ZipFile.java line 1145:
> 
>> 1143:     static record EntryPos(String name, int pos) {}
>> 1144: 
>> 1145:     // Implementation note: This class is be thread safe.
> 
> Suggestion:
> 
>     // Implementation note: This class is thread safe.

Fixed

> src/java.base/share/classes/java/util/zip/ZipFile.java line 1432:
> 
>> 1430:          * where a ZIP file is re-opened after it has been modified).
>> 1431:          * - The Charset, that was provided when constructing a ZipFile instance,
>> 1432:          * for reading non-UTF-8 entry names and comments.
> 
> I think it would be sufficient to say "The Charset that was provided when constructing the ZipFile instance". Any non-UTF8-ness is better explained  elsewhere.

Fixed

> src/java.base/share/classes/java/util/zip/ZipFile.java line 1438:
> 
>> 1436:             private final BasicFileAttributes attrs;
>> 1437:             private final File file;
>> 1438:             // the Charset to be used for processing non-UTF-8 entry names in the ZIP file
> 
> Similarly this could just say "The Charset that was provided when constructing the ZipFile instance"

Yes, I think your suggested comment is good. I've updated the PR to use this.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/23986#discussion_r2068530939
PR Review Comment: https://git.openjdk.org/jdk/pull/23986#discussion_r2068531394
PR Review Comment: https://git.openjdk.org/jdk/pull/23986#discussion_r2068531636
PR Review Comment: https://git.openjdk.org/jdk/pull/23986#discussion_r2068531991
PR Review Comment: https://git.openjdk.org/jdk/pull/23986#discussion_r2068533150