On Sun, 23 Mar 2025 09:28:38 GMT, Jaikiran Pai <j...@openjdk.org> wrote:
>> Can I please get a review of this change which proposes to fix an issue >> `java.util.zip.ZipFile` which would cause failures when multiple instances >> of `ZipFile` using non-UTF8 `Charset` were operating against the same >> underlying ZIP file? This addresses >> https://bugs.openjdk.org/browse/JDK-8347712. >> >> ZIP file specification allows for ZIP entries to mark a `UTF-8` flag to >> indicate that the entry name and comment are encoded using UTF8. A >> `java.util.zip.ZipFile` can be constructed by passing it a `Charset`. This >> `Charset` (which defaults to UTF-8) gets used for decoding entry names and >> comments for non-UTF8 entries. >> >> The internal implementation of `ZipFile` uses a `ZipCoder` (backed by >> `java.nio.charset.CharsetEncoder/CharsetDecoder` instance) for the given >> `Charset`. Except for UTF8 `ZipCoder`, other `ZipCoder`s are not thread safe. >> >> The internal implementation of `ZipFile` maintains a cache of >> `ZipFile$Source`. A `Source` corresponds to the underlying ZIP file and >> during construction, uses a `ZipCoder` for parsing the ZIP entries and once >> constructed holds on to the parsed ZIP structure. Multiple instances of a >> `ZipFile` which all correspond to the same ZIP file on the filesystem, share >> a single instance of `Source` (after the `Source` has been constructed and >> cached). Although `ZipFile` instances aren't expected to be thread-safe, the >> fact that multiple different instances of `ZipFile` could be sharing the >> same instance of `Source` in concurrent threads, mandates that the `Source` >> must be thread-safe. >> >> In Java 15, we did a performance optimization through >> https://bugs.openjdk.org/browse/JDK-8243469. As part of that change, we >> started holding on to the `ZipCoder` instance (corresponding to the >> `Charset` provided during `ZipFile` construction) in the `Source`. This >> stored `ZipCoder` was then used for `ZipFile` operations when working with >> the ZIP entries. As noted previously, any non-UTF8 `ZipCoder` is not >> thread-safe and as a result, any usages of `ZipCoder` in the `Source` makes >> `Source` not thread-safe too. That effectively violates the requirement that >> `Source` must be thread-safe to allow for its usage in multiple different >> `ZipFile` instances concurrently. This then causes `ZipFile` usages to fail >> in unexpected ways like the one shown in the linked >> https://bugs.openjdk.org/browse/JDK-8347712. >> >> The commit in this PR addresses the issue by not maintaining `ZipCoder` as a >> instance field of `Source`. Instead the `ZipCoder` is now mainta... > > 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 I think the root cause is the stateless nature of `ZipFile.Source` and the stateful nature of `ZipCoder`. Exposing the UTF8 coder as stateless in class-level docs IMO increases complexity; it is just an implementation artifact that allows `ZipCoder.get` to return the same singleton instead of recreating a new instance to carry states. We shouldn't promote the storage of any `ZipCoder` in `Source`. 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. 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. Should we comment that this class has no observable state in addition to being thread safe? ------------- PR Review: https://git.openjdk.org/jdk/pull/23986#pullrequestreview-2708904844 PR Review Comment: https://git.openjdk.org/jdk/pull/23986#discussion_r2009327538 PR Review Comment: https://git.openjdk.org/jdk/pull/23986#discussion_r2009328334
