RFR: 8350880: (zipfs) Add support for read-only zip file systems [v4]

[Daniel Fuchs]

On Mon, 19 May 2025 12:24:28 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

>> Hello David, I had another look at this code and after going through it, it looked like `readOnly` field can in fact be made `final` because of the refactoring changes that you did in this PR. I checked out your latest PR locally and here's the additional change I did:
>> 
>> 
>> diff --git a/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java b/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java
>> index e2fddd96fe8..f54b5360ac5 100644
>> --- a/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java
>> +++ b/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java
>> @@ -110,8 +110,7 @@ class ZipFileSystem extends FileSystem {
>>      private final Path zfpath;
>>      final ZipCoder zc;
>>      private final ZipPath rootdir;
>> -    // Starts in readOnly (safe mode), but might be reset at the end of initialization.
>> -    private boolean readOnly = true;
>> +    private final boolean readOnly;
>>  
>>      // default time stamp for pseudo entries
>>      private final long zfsDefaultTimeStamp = System.currentTimeMillis();
>> @@ -227,11 +226,6 @@ static ZipAccessMode from(Object value) {
>>  
>>          // Determining a release version uses 'this' instance to read paths etc.
>>          Optional<Integer> multiReleaseVersion = determineReleaseVersion(env);
>> -
>> -        // Set the version-based lookup function for multi-release JARs.
>> -        this.entryLookup =
>> -                multiReleaseVersion.map(this::createVersionedLinks).orElse(Function.identity());
>> -
>>          // We only allow read-write zip/jar files if they are not multi-release
>>          // JARs and the underlying file is writable.
>>          this.readOnly = forceReadOnly || multiReleaseVersion.isPresent() || !Files.isWritable(zfpath);
>> @@ -241,6 +235,10 @@ static ZipAccessMode from(Object value) {
>>                      : "the ZIP file is not writable";
>>              throw new IOException(reason);
>>          }
>> +        // Set the version-based lookup function for multi-release JARs.
>> +        this.entryLookup =
>> +                multiReleaseVersion.map(this::createVersionedLinks).orElse(Function.identity());
>> +
>>      }
>>  
>>      /**
>> 
>> 
>> 
>> 
>> With the refactoring changes you have done so far, we are now able to determine the ultimate value for `readOnly` before anything in the construction of `ZipFileSystem` would need access to it. Does this additional change look reasonable to you? I haven't run any tests against this change.
>> 
>> Making it `final` and having...
>
> On second thought, may be this isn't enough. I see that I missed the fact that `determineReleaseVersion()` requires access to `this`. Please leave this in the current form that you have in your PR. I need a few more moments to consider if anything needs to be changed.

final members are good! Alternatively if making it final is not an option you could consider replacing:

private boolean readOnly;

with

private boolean readWrite;

I  looked at where `readOnly` was used and there are not that many places, so inverting the flag might not be too intrusive.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2109210071