8354221: (zipfs) Tidy initialization of jdk.nio.zipfs.ZipFileSystem #24555

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Draft

david-beaumont wants to merge 7 commits into openjdk:master from david-beaumont:jdk_8354221

Contributor

david-beaumont commented Apr 9, 2025 •

edited by openjdk bot

Loading

Tidy ZipFileSystem initialization to make it easier to reason about the connection between multi-release JARs and the setting of the read-only flag.
Inlined the mutable final static field DEFAULT_PERMISSIONS, which was only used once (having a mutable static field seems like something to generally be avoided).
Make the defaultPermissions field non-mutable and avoid copying a Set every time it's read.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8354221: (zipfs) Tidy initialization of jdk.nio.zipfs.ZipFileSystem (Enhancement - P4)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/24555/head:pull/24555
$ git checkout pull/24555

Update a local copy of the PR:
$ git checkout pull/24555
$ git pull https://git.openjdk.org/jdk.git pull/24555/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 24555

View PR using the GUI difftool:
$ git pr show -t 24555

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/24555.diff

bridgekeeper bot commented Apr 9, 2025

👋 Welcome back david-beaumont! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk bot commented Apr 9, 2025

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

openjdk bot commented Apr 9, 2025

@david-beaumont The following labels will be automatically applied to this pull request:

core-libs
nio

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

openjdk bot added nio core-libs labels

david-beaumont commented

View reviewed changes

Contributor Author

david-beaumont left a comment

Explanatory thoughts and a couple of questions.

src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java

@@ @@ -104,7 +103,8 @@ class ZipFileSystem extends FileSystem { @@
                   private final Path zfpath;
                   final ZipCoder zc;
                   private final ZipPath rootdir;
-                  private boolean readOnly; // readonly file system, false by default
+                  // Start readOnly (safe mode) and maybe reset at end of initialization.
+                  private boolean readOnly = true;

Contributor Author

david-beaumont Apr 9, 2025

Seems more sensible to start in read-only mode. There's no sign this was ever an issue, but it feels safer in general.

src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java Outdated

@@ @@ -129,6 +129,7 @@ class ZipFileSystem extends FileSystem { @@
                   final boolean supportPosix;
                   private final UserPrincipal defaultOwner;
                   private final GroupPrincipal defaultGroup;
+                  // Unmodifiable set.

Contributor Author

david-beaumont Apr 9, 2025

Or I could just rename it to unmodifiableDefaultPosixPermissions which would let me remove 2 comments (here and where it's returned).

Member

liach Apr 9, 2025

I think we don't usually indicate a collection is unmodifiable in the field names - a comment suffices, and in fact, I think we promote using unmodifiable collections as the default for non-mutating data now.

src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java Outdated

@@ @@ -147,7 +148,7 @@ class ZipFileSystem extends FileSystem { @@
                       this.supportPosix = isTrue(env, PROPERTY_POSIX);
                       this.defaultOwner = supportPosix ? initOwner(zfpath, env) : null;
                       this.defaultGroup = supportPosix ? initGroup(zfpath, env) : null;
-                      this.defaultPermissions = supportPosix ? initPermissions(env) : null;
+                      this.defaultPermissions = supportPosix ? Collections.unmodifiableSet(initPermissions(env)) : null;

Contributor Author

david-beaumont Apr 9, 2025

Wrap it on setting the field rather than in the helper so it's clearly done unconditionally.

src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java

@@ @@ -180,7 +178,22 @@ class ZipFileSystem extends FileSystem { @@
                       this.provider = provider;
                       this.zfpath = zfpath;
-                      initializeReleaseVersion(env);
+                      // Determining a release version uses 'this' instance to read paths etc.

Contributor Author

david-beaumont Apr 9, 2025

The bulk of this change is moving the (re)setting of these fields down to the end of initialization, and now doing it in the constructor itself. Even though the helper functions are not static (they could be if needed) they no longer mutate the instance.

src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java Outdated

                       if (o == null) {
-                          return DEFAULT_PERMISSIONS;
+                          return PosixFilePermissions.fromString("rwxrwxrwx");

Contributor Author

david-beaumont Apr 9, 2025

Inlining the default permission seems fine (used once, matches the comment 3 lines above).

Member

liach Apr 9, 2025

We can probably use EnumSet.allOf here - same enum set remark for the hash set usage below. The old field allows sharing the unmodified set instance, but enum set is cheap so creating should be fine.

src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java

@@ @@ -1383,33 +1391,33 @@ private void removeFromTree(IndexNode inode) { @@
                    * Checks if the Zip File System property "releaseVersion" has been specified. If it has,
                    * use its value to determine the requested version. If not use the value of the "multi-release" property.
                    */
-                  private void initializeReleaseVersion(Map<String, ?> env) throws IOException {
+                  private Optional<Integer> determineReleaseVersion(Map<String, ?> env) throws IOException {

Contributor Author

david-beaumont Apr 9, 2025

I should probably change the comment above a bit. Thoughts on rewording welcome.

src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java Outdated

                    * Then wrap the map in a function that getEntry can use to override root
                    * entry lookup for entries that have corresponding versioned entries.
                    */
-                  private void createVersionedLinks(int version) {
+                  private Function<byte[], byte[]> createVersionedLinks(int version) {

Contributor Author

david-beaumont Apr 9, 2025

Or return Optional<Function<...>> perhaps. Thoughts welcome.
Comment above can probably be changed too.

Member

liach Apr 9, 2025

I would return a UnaryOperator<byte[]> just because the type name is shorter :)

src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java Outdated

@@ @@ -3551,7 +3556,8 @@ public GroupPrincipal group() { @@
                       @Override
                       public Set<PosixFilePermission> permissions() {
-                          return storedPermissions().orElse(Set.copyOf(defaultPermissions));
+                          // supportPosix ==> (defaultPermissions != null) and it's unmodifiable.

Contributor Author

david-beaumont Apr 9, 2025

I'd be happy to drop the comment if I renamed the field to unmodifiableDefaultPosixPermissions.

Member

liach Apr 9, 2025

Hmm, PosixFileAttributes requires this returned set to be modifiable. Seems the existing implementation already violates the specification, but we might just adjust the specs to say that the returned set, if modifiable, does not affect the actual permission of the attributes.

liach reviewed

View reviewed changes

src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java Outdated

@@ @@ -3551,7 +3556,8 @@ public GroupPrincipal group() { @@
                       @Override
                       public Set<PosixFilePermission> permissions() {
-                          return storedPermissions().orElse(Set.copyOf(defaultPermissions));
+                          // supportPosix ==> (defaultPermissions != null) and it's unmodifiable.

Member

liach Apr 9, 2025

Hmm, PosixFileAttributes requires this returned set to be modifiable. Seems the existing implementation already violates the specification, but we might just adjust the specs to say that the returned set, if modifiable, does not affect the actual permission of the attributes.

src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java Outdated

                    * Then wrap the map in a function that getEntry can use to override root
                    * entry lookup for entries that have corresponding versioned entries.
                    */
-                  private void createVersionedLinks(int version) {
+                  private Function<byte[], byte[]> createVersionedLinks(int version) {

Member

liach Apr 9, 2025

I would return a UnaryOperator<byte[]> just because the type name is shorter :)

src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java Outdated

                       if (o == null) {
-                          return DEFAULT_PERMISSIONS;
+                          return PosixFilePermissions.fromString("rwxrwxrwx");

Member

liach Apr 9, 2025

We can probably use EnumSet.allOf here - same enum set remark for the hash set usage below. The old field allows sharing the unmodified set instance, but enum set is cheap so creating should be fine.

src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java Outdated

@@ @@ -129,6 +129,7 @@ class ZipFileSystem extends FileSystem { @@
                   final boolean supportPosix;
                   private final UserPrincipal defaultOwner;
                   private final GroupPrincipal defaultGroup;
+                  // Unmodifiable set.

Member

liach Apr 9, 2025

I think we don't usually indicate a collection is unmodifiable in the field names - a comment suffices, and in fact, I think we promote using unmodifiable collections as the default for non-mutating data now.

src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java

+                      // It requires 'entryLookup' and 'readOnly' to have safe defaults (which
+                      // is why they are the only non-final fields), and it requires that the
+                      // inode map has been initialized.
+                      Optional<Integer> multiReleaseVersion = determineReleaseVersion(env);

Member

liach Apr 9, 2025

This call still feels risky - this is probably a "this escape" and can cause troubles if isMultiReleaseJar uses final fields not yet initialized below.

Contributor Author

david-beaumont Apr 10, 2025

Thanks for pointing out the mutable permissions set thing. I guess we'll just need to make a copy to return to the user every time (or explicitly continue to violate the spec). EnumSet is super cheap, but I'll change the internal field to be an EnumSet so we don't have any risk of creating a larger data structure and then copying that every time.

As regards the "this escape" comment. How can it escape?

The code called is private and reads from the inode table with the default file mapping function (and this is still better than the existing code because it waits until everything else is set before doing the read).

liach reviewed

View reviewed changes

src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java Outdated

-                          return storedPermissions().orElse(Set.copyOf(defaultPermissions));
+                          // supportPosix ==> (defaultPermissions != null).
+                          assert defaultPermissions != null;
+                          return storedPermissions().orElse(defaultPermissions.clone());

Member

liach Apr 10, 2025

Suggested change

      
                        return storedPermissions().orElse(defaultPermissions.clone());
          
                        return storedPermissions().orElseGet(defaultPermissions::clone);

So we don't clone if stored permissions is present.

Contributor Author

david-beaumont Apr 10, 2025

doh Excellent spot, thanks!

david-beaumont commented

View reviewed changes

test/jdk/jdk/nio/zipfs/TestPosix.java

+                      }
+                  }
+                  private Set<PosixFilePermission> getFilePermissions(

Contributor Author

david-beaumont Apr 10, 2025

I pulled out the permission getting logic so I can get permissions several times.
I even think if reads a bit nicer than before tbh (less nesting and a clearer mapping in the switch statement).

david-beaumont force-pushed the jdk_8354221 branch from d3d07bb to 61389ad Compare

April 14, 2025 21:58

david-beaumont added 6 commits

April 16, 2025 13:18


          Tidying initialization.

7f46da2


          Minor comment changes.

cc666ff


          Making default permissions an EnumSet.

* Making default permissions always be an enum set and cloning it on return (it must be mutable, contrary to what the existing code does).


          Moving comment re. EnumSet.

8f51551


          Adding tests for mutability.

03b417a

* Add tests and fix daft mistake with initPermissions().
* Function -> UnaryOperator (better describes the intent).


          Use method reference for clone() to avoid copy-on-call.

d027149

david-beaumont force-pushed the jdk_8354221 branch from 61389ad to d027149 Compare

April 16, 2025 22:22


          Merge branch 'master' into jdk_8354221/squashed

82008ad

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels