On Fri, 8 Dec 2023 10:18:18 GMT, Jaikiran Pai <j...@openjdk.org> wrote:
>> 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
> + "@" + Intege...
Hi Jai,
Thank you for the detailed response and suggested alternative.
My intentions were to get the file name by leveraging the existing code without
introducing an additional field if possible, but I did not realize the `NPE` it
would cause when closing the ZipFile as you demonstrated.
I appreciate your guidance, updated the PR to reflect your suggestions. I don't
see how this would cause any issues with tests as you stated, but checked tiers
1-3 and all is good there.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/16643#discussion_r1421964393
