On Mon, 19 May 2025 11:41:16 GMT, David Beaumont <d...@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
