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

[Jaikiran Pai]

On Mon, 19 May 2025 11:41:16 GMT, David Beaumont <duke at openjdk.org> wrote:

>> src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java line 114:
>> 
>>> 112:     private final ZipPath rootdir;
>>> 113:     // Starts in readOnly (safe mode), but might be reset at the end of initialization.
>>> 114:     private boolean readOnly = true;
>> 
>> If `readOnly` gets used by some code when a `ZipFileSystem` instance is being constructed (which is why I believe this can't be a `final` field), then I think we should not change this value to `true`. In other words, would this change now have a chance of introducing a `ReadOnlyFileSystemException` when constructing the `ZipFileSystem` whereas before it wouldn't?
>
> ""would this change now have a chance of introducing a ReadOnlyFileSystemException when constructing the ZipFileSystem whereas before it wouldn't?""
> 
> If there was ever a "ReadOnlyFileSystemException" when initializing the ZipFileSystem, we have a very serious bug already. Since we already have the opening of Zip working for cases where the underlying file is read-only in the file system, it has to be true that no attempt is made to modify the file contents.
> 
> While I accept that this new code now calls out to functions with this flag in a different state to before, I believe this is an absolute improvement since:
> 
> 1. It is not acceptable to say to users "we support a read-only mode" if during initialization we might modify the file in any way (even including changing last-modification times etc.).
> 
> 2. All the evidence points to whichever operations are being done during init as being fundamentally "read-only" (both due to it working on read-only zip files, and there being no conceptual need to modify anything during setup).
> 
> I'll happily do a thorough audit of everything which could be affected by this change if that will give you confidence, but I would not want to change this code back to its default "read-write until stated otherwise" behaviour.

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 it not accessed until the ultimate value is set in the constructor would get us past any of the concerns we may have about the "initial" value of `readOnly`.

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

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