Re: RFR: JDK-8319626: Override toString() for ZipFile [v7]
On Mon, 11 Dec 2023 05:44:38 GMT, Justin Lu 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@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
Re: RFR: JDK-8319626: Override toString() for ZipFile [v7]
On Fri, 8 Dec 2023 10:18:18 GMT, Jaikiran Pai 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
Re: RFR: JDK-8319626: Override toString() for ZipFile [v7]
On Fri, 1 Dec 2023 21:32:57 GMT, Justin Lu 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
Re: RFR: JDK-8319626: Override toString() for ZipFile [v7]
> 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 - Changes: - all: https://git.openjdk.org/jdk/pull/16643/files - new: https://git.openjdk.org/jdk/pull/16643/files/21762cd2..159c4c8f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16643=06 - incr: https://webrevs.openjdk.org/?repo=jdk=16643=05-06 Stats: 4 lines in 1 file changed: 0 ins; 4 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/16643.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16643/head:pull/16643 PR: https://git.openjdk.org/jdk/pull/16643
