On Mon, 19 May 2025 12:22:11 GMT, Jaikiran Pai <j...@openjdk.org> wrote:
>> ""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 t...
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.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2095591964
