On Fri, 1 Dec 2023 21:32:57 GMT, Justin Lu <j...@openjdk.org> wrote:
>> Please review this PR and [CSR](https://bugs.openjdk.org/browse/JDK-8319982)
>> which overrides and provides an implementation of `toString()` in
>> _java.util.zip.ZipFile_ (and by extension, _java.util.jar.JarFile_).
>
> Justin Lu has updated the pull request incrementally with one additional
> commit since the last revision:
>
> drop additional specification
src/java.base/share/classes/java/util/zip/ZipFile.java line 494:
> 492: @Override
> 493: public String toString() {
> 494: return res.zsrc.key.file.getName()
Hello Justin, relying on `res.zsrc.key.file` to get to the file name can cause
troubles. For example, consider this usage (which I ran in jshell) with this
proposed change in this PR:
jshell> import java.util.zip.*
jshell> ZipFile zf = new ZipFile("/tmp/workdir.zip")
zf ==> workdir.zip@34c45dca
jshell> zf.close()
jshell> zf.toString()
| Exception java.lang.NullPointerException: Cannot read field "key" because
"this.res.zsrc" is null
| at ZipFile.toString (ZipFile.java:494)
| at (#4:1)
What I do here is that I call `ZipFile.toString()` after I (intentionally)
close the `ZipFile`. The `toString()` call leads to `NullPointerException`
because on closing the `ZipFile` instance we clean up the resource it holds on
to.
I think what we can do here is, something like (the following diff was
generated on top of the current state of this PR):
diff --git a/src/java.base/share/classes/java/util/zip/ZipFile.java
b/src/java.base/share/classes/java/util/zip/ZipFile.java
index 67a5e65089f..2b6996294e1 100644
--- a/src/java.base/share/classes/java/util/zip/ZipFile.java
+++ b/src/java.base/share/classes/java/util/zip/ZipFile.java
@@ -95,7 +95,8 @@
*/
public class ZipFile implements ZipConstants, Closeable {
- private final String name; // zip file name
+ private final String filePath; // zip file path
+ private final String fileName; // name of the file
private volatile boolean closeRequested;
// The "resource" used by this zip file that needs to be
@@ -245,7 +246,8 @@ public ZipFile(File file, int mode, Charset charset) throws
IOException
}
Objects.requireNonNull(charset, "charset");
- this.name = name;
+ this.filePath = name;
+ this.fileName = file.getName();
long t0 = System.nanoTime();
this.res = new CleanableResource(this, ZipCoder.get(charset), file,
mode);
@@ -483,7 +485,7 @@ public int available() throws IOException {
* @return the path name of the ZIP file
*/
public String getName() {
- return name;
+ return filePath;
}
/**
@@ -491,7 +493,7 @@ public String getName() {
*/
@Override
public String toString() {
- return res.zsrc.key.file.getName()
+ return this.fileName
+ "@" + Integer.toHexString(System.identityHashCode(this));
}
The above example usage now works even after I close the ZipFile. I haven't run
other tests, but given that this doesn't change any APIs or their
implementation, I think the tests would run fine too.
Do you see any issues with this?
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/16643#discussion_r1420232060
