On Thu, 26 Feb 2026 13:39:51 GMT, Eirik Bjørsnøs <[email protected]> wrote:
>> Please review this cleanup PR where we simplify synchronization on ZipFile's
>> inflater cache.
>>
>> Currently, the `ZipFile.CleanableResource.inflaterCache` field is non-final,
>> is used in synchronization and is set to `null` to indicate a closed
>> inflater cache. This complicates the state considerations for
>> synchronization, requiring double-checking that the cache does not close
>> under us. Generally, correctness of synchronizing on a non-final field which
>> can also be null is hard to reason about.
>>
>> This PR marks the `inflaterCache` field as `final` and introduces a boolean
>> flag field to model the closed state explicitly. This allows synchronization
>> to be simplified, double-checking to be removed and the closed state to be
>> more obvious. If we want the future ZipFile to be less mutable, this is one
>> step in that direction.
>>
>> Cleanup refactoring, `noreg-cleanup`
>
> Eirik Bjørsnøs has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Reduce memory retained by a closed ZipFile
Old code ensured getInflater can fail properly when inflaterCache is null. This
safety is lost in your refactor.
src/java.base/share/classes/java/util/zip/ZipFile.java line 736:
> 734: if (inflaterCacheClosed) {
> 735: // inflaters cache already closed - just end it.
> 736: inf.end();
In your new code, we always need to get the inflaterCache monitor to end an
inflator when the cache is closed. Could be a regression.
-------------
PR Review: https://git.openjdk.org/jdk/pull/29937#pullrequestreview-3861810399
PR Review Comment: https://git.openjdk.org/jdk/pull/29937#discussion_r2859749076
