RFR: JDK-8319626: Override toString() for ZipFile [v7]
Jaikiran Pai
jpai at openjdk.org
Tue Dec 12 09:42:39 UTC 2023
On Mon, 11 Dec 2023 05:44:38 GMT, Justin Lu <jlu at openjdk.org> wrote:
>> 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 at 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
>> publi...
>
> 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.
Hello Justin, thank you for doing the update and running the tests. This latest state of the PR (commit `4ba2bd47`) looks good to me. I have no other review comments.
Please wait for an additional review from someone else before integrating.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/16643#discussion_r1423725222
More information about the core-libs-dev
mailing list