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

[Jaikiran Pai]

On Mon, 24 Mar 2025 15:01:09 GMT, Eirik Bjørsnøs <eirbjo at openjdk.org> wrote:

>> src/java.base/share/classes/java/util/zip/ZipCoder.java line 44:
>> 
>>> 42:  * <p>
>>> 43:  * The {@code ZipCoder} for UTF-8 charset is thread safe, {@code ZipCoder}
>>> 44:  * for other charsets require external synchronization.
>> 
>> I think the "thread safe" feature is already implied by the comment on UTF8: "Encoding/decoding is stateless". So I recommend just mentioning that a ZipCoder may carry states, and a ZipCoder obtained from `ZipCoder.get` should only be used locally.
>
> I agree with @liach here. The fact that one ZipCoder is stateless and thread-safe is a class-internal implementation detail and not information which any call site of this API makes use of.

> So I recommend just mentioning that a ZipCoder may carry states, and a ZipCoder obtained from ZipCoder.get should only be used locally.

The `UTF8ZipCoder` may not just be accessed through the `ZipCoder.UTF8` field. It could even be obtained through `ZipCoder.get(UTF_8)`. So the ZipCode returned by `ZipCoder.get()` can still be used concurrently (if it is for UTF-8).

> The fact that one ZipCoder is stateless and thread-safe is a class-internal implementation detail and not information which any call site of this API makes use of.

The `ZipCoder` itself is an internal implementation detail (it's a package protected class), so the javadoc that we are discussing here won't be published by the `java.base` module. I believe that the thread safety aspect of the ZipCoder instances, even if it for some specific charsets, is an important detail even for internal call sites to be aware of. In fact, the current issue that we are fixing shows that this detail might have helped prevent this issue.

I think the brief comment that we have added here, in its current form, is good enough. So unless either of you or others feel strongly about it, I would like to keep it in its current form.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/23986#discussion_r2068550089