Skip to content
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.

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

8269886: Inaccurate error message for compressed hprof test #4685

Closed
wants to merge 7 commits into from

Conversation

linzang
Copy link
Contributor

@linzang linzang commented Jul 6, 2021

The current implementation of hprof Reader for testing always prompts "Can not decompress the compressed hprof file" when there is error for testing gzipped heap dump. This is inaccurate if the gzipped file was decompressed successfully but the hprof file format is incorrect. So the inaccurate error message could be misleading for issue analysis.

This trivial PR refine the error message by simply print "Can not get stack trace from the compressed hprof file", the underlying exception from GZIPInputStream() or HprofReader() would give accurate error info.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8269886: Inaccurate error message for compressed hprof test

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/4685/head:pull/4685
$ git checkout pull/4685

Update a local copy of the PR:
$ git checkout pull/4685
$ git pull https://git.openjdk.java.net/jdk pull/4685/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4685

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/4685.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jul 6, 2021

👋 Welcome back lzang! 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 openjdk bot added the rfr label Jul 6, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jul 6, 2021

@linzang The following labels will be automatically applied to this pull request:

  • hotspot-runtime
  • serviceability

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 openjdk bot added serviceability hotspot-runtime labels Jul 6, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Jul 6, 2021

Webrevs

@dholmes-ora
Copy link
Member

@dholmes-ora dholmes-ora commented Jul 6, 2021

The existing message is a generic message covering the general operation of the whole try block. It seems far more appropriate than your new message, which only seems to apply to the final step. ???

David

@linzang
Copy link
Contributor Author

@linzang linzang commented Jul 6, 2021

Hi David,
Thanks for review and comments.

The existing message is a generic message covering the general operation of the whole try block. It seems far more appropriate than your new message, which only seems to apply to the final step. ???

IMO, the whole method named getStack() is used to get the stack trace for the hprof file. And the try block could throw exceptions by serveral reasons as decribed, so IMHO printing "Can not decompress the compressed hprof file" is not accurate. The proposed change is to print a generic error message that the getStack() fail when processing compressed hprof. And the extra 'e' argument in the changed line would give detailed reason.

So may be change error message to "Can not process the compressed hprof file" is better? what do you think?

BRs,
Lin

@linzang
Copy link
Contributor Author

@linzang linzang commented Jul 6, 2021

Hi @dholmes-ora
Thanks for your suggestion! I have update the PR which split the try block into two seperate ones to process different exceptions.

BRs,
Lin

Copy link
Member

@dholmes-ora dholmes-ora left a comment

These changes are fine in themselves.

This code is not very well written though - the file streams do not get closed and the decompressed output file doesn't seem to get deleted when I would expect.

Thanks,
David

@openjdk
Copy link

@openjdk openjdk bot commented Jul 6, 2021

@linzang This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8269886: Inaccurate error message for compressed hprof test

Reviewed-by: dholmes, cjplummer

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 54 new commits pushed to the master branch:

  • e54585b: 8268363: AArch64: Implement string_indexof_char intrinsic in SVE
  • 270fbcb: Merge
  • c812bbb: 8269929: (test) Add diagnostic info to ProceessBuilder/Basic.java for unexpected output
  • 6000950: 8269185: Directories in /opt/runtimepackagetest and /path/to/jdk-17 are different
  • 1f2bf1d: 8269879: [PPC64] C2: Math.rint intrinsic uses wrong rounding mode
  • 7fcd5ca: 8266036: class file for sun.misc.Contended not found
  • a49b1dc: 8269772: [macos-aarch64] test compilation failed with "SocketException: No buffer space available"
  • 820f290: 8268859: jshell throws exception while parsing illegal "case true"
  • 815e4af: 8269802: javac fails to compile nested pattern matching switches
  • 2daf39a: 8269830: SA's vm object vtable matching code sometimes matches on incorrect type
  • ... and 44 more: https://git.openjdk.java.net/jdk/compare/371d996a892fc6fbf82110a1ca5b3d64a801b6fc...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@dholmes-ora, @plummercj) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready label Jul 6, 2021
@linzang
Copy link
Contributor Author

@linzang linzang commented Jul 6, 2021

Hi David,
Thanks for reviewing!

This code is not very well written though - the file streams do not get closed and the decompressed output file doesn't seem to get deleted when I would expect.

Agree. I will create a seperate issue to fix it.

BRs,
Lin

@linzang
Copy link
Contributor Author

@linzang linzang commented Jul 6, 2021

/integrate

@openjdk openjdk bot added the sponsor label Jul 6, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jul 6, 2021

@linzang
Your change (at version 76a69c3) is now ready to be sponsored by a Committer.

@plummercj
Copy link
Contributor

@plummercj plummercj commented Jul 6, 2021

You need 2 reviews. Please don't integrate yet. I'll provide more comments shortly.

@plummercj
Copy link
Contributor

@plummercj plummercj commented Jul 6, 2021

After decompressing, no error or exception is thrown if MAGIC_NUMBER is not found. It just silently fails to print the stack trace.

Please use "cannot" instead of "can not". Both are grammatically correct, but "cannot" is what is normally used.

@openjdk openjdk bot removed the sponsor label Jul 7, 2021
@linzang
Copy link
Contributor Author

@linzang linzang commented Jul 7, 2021

Hi @plummercj ,

Thanks for reviewing, I have updated the PR based on your comments, and remove unreachable code introduced by the updated code.

BRs,
Lin

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Either deleting the out file needs to be handled correctly in all cases or it should left as-is and fixed in a different RFE that also handles stream closing *try-with-resources?) etc. to clean up the code.

Otherwise the actual try/catch changes seem okay.

Thanks,
David

test/lib/jdk/test/lib/hprof/parser/Reader.java Outdated Show resolved Hide resolved
@openjdk openjdk bot removed the ready label Jul 7, 2021
@linzang
Copy link
Contributor Author

@linzang linzang commented Jul 7, 2021

Hi @dholmes-ora

Either deleting the out file needs to be handled correctly in all cases or it should left as-is and fixed in a different RFE that also handles stream closing *try-with-resources?) etc. to clean up the code.

I have created an issue in JBS (https://bugs.openjdk.java.net/browse/JDK-8269909) to use the try-with-resource for stream closing, and will submit a PR shortly.

For out.delete(), I prefer to fix it in this PR as the introduction of throwing exception in my last update makes the original out.delete() unreachable, so I have to add logic to handle it. I will update this PR with a finally block in a minute.

Thanks,
Lin

Copy link
Member

@dholmes-ora dholmes-ora left a comment

I would have added a single try/finally block that encompasses all the code after:

File out = new File(deCompressedFile);

but what you have also does the job.

Thanks,
David

@openjdk openjdk bot added the ready label Jul 7, 2021
@plummercj
Copy link
Contributor

@plummercj plummercj commented Jul 7, 2021

There is still an inconsistency exception handling. For the outer bad MAGIC_NUMBER handling you have:

186 throw new IOException("Unrecognized magic number: " + i);

But the decompressed handling results in:

178 throw new IOException("Unrecognized magic number of decompressed data: " + i);

Which is caught and wrapped by:

181 throw new IOException("Cannot get stack trace from the compressed hprof file", e);

Also, nothing is wrapping exceptions thrown by the following code:

143 HprofReader r
144 = new HprofReader(heapFile, in, dumpNumber,
145 true, debugLevel);
146 r.read();
147 return r.printStackTraces();

But any exception thrown by the following code is wrapped and rethrown at line 181:

172 HprofReader r
173 = new HprofReader(deCompressedFile, in2, dumpNumber,
174 true, debugLevel);
175 r.read();
176 return r.printStackTraces();

@linzang
Copy link
Contributor Author

@linzang linzang commented Jul 7, 2021

Hi @plummercj,
Thanks for point it out! I think the throw at line 181 could be removed and the new = HprofReader(); r.read() code should be in try block consistently in both situation (compressed/uncompressed). I will make the change.
-Lin

@linzang
Copy link
Contributor Author

@linzang linzang commented Jul 7, 2021

Hi @plummercj,
I just found that maybe there is no need to catch exception from HprofReader in this method, just keep it as what it originally did - throw any exception from HprofReader directly. The information from HprofReader's exception is enough for root cause analysis in this case.
I will test it and make the change.

BRs,
Lin

Copy link
Contributor

@plummercj plummercj left a comment

Looks good other than cleaning up the exception message.

test/lib/jdk/test/lib/hprof/parser/Reader.java Outdated Show resolved Hide resolved
@linzang
Copy link
Contributor Author

@linzang linzang commented Jul 8, 2021

I will mark this PR with integrate.
Thanks Chris and David for your reviewing!

BRs,
Lin

@linzang
Copy link
Contributor Author

@linzang linzang commented Jul 8, 2021

/integrate

Copy link
Member

@dholmes-ora dholmes-ora left a comment

LGTM.

@openjdk openjdk bot added the sponsor label Jul 8, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jul 8, 2021

@linzang
Your change (at version d91ffd8) is now ready to be sponsored by a Committer.

@dholmes-ora
Copy link
Member

@dholmes-ora dholmes-ora commented Jul 8, 2021

/sponsor

@openjdk
Copy link

@openjdk openjdk bot commented Jul 8, 2021

Going to push as commit 4fbcce1.
Since your change was applied there have been 56 commits pushed to the master branch:

  • a96012f: 8269803: G1: remove unnecessary NoRefDiscovery
  • 4e18ec2: 8269993: [Test]: java/net/httpclient/DigestEchoClientSSL.java contains redundant @run tags
  • e54585b: 8268363: AArch64: Implement string_indexof_char intrinsic in SVE
  • 270fbcb: Merge
  • c812bbb: 8269929: (test) Add diagnostic info to ProceessBuilder/Basic.java for unexpected output
  • 6000950: 8269185: Directories in /opt/runtimepackagetest and /path/to/jdk-17 are different
  • 1f2bf1d: 8269879: [PPC64] C2: Math.rint intrinsic uses wrong rounding mode
  • 7fcd5ca: 8266036: class file for sun.misc.Contended not found
  • a49b1dc: 8269772: [macos-aarch64] test compilation failed with "SocketException: No buffer space available"
  • 820f290: 8268859: jshell throws exception while parsing illegal "case true"
  • ... and 46 more: https://git.openjdk.java.net/jdk/compare/371d996a892fc6fbf82110a1ca5b3d64a801b6fc...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Jul 8, 2021
@openjdk openjdk bot added integrated and removed ready rfr sponsor labels Jul 8, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jul 8, 2021

@dholmes-ora @linzang Pushed as commit 4fbcce1.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@linzang linzang deleted the reader branch Jul 8, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Jul 8, 2021

Mailing list message from David Holmes on hotspot-runtime-dev:

Hi Lin,

On 6/07/2021 1:33 pm, Lin Zang wrote:

On Tue, 6 Jul 2021 02:55:37 GMT, Lin Zang <lzang at openjdk.org> wrote:

The current implementation of hprof Reader for testing always prompts "Can not decompress the compressed hprof file" when there is error for testing gzipped heap dump. This is inaccurate if the gzipped file was decompressed successfully but the hprof file format is incorrect. So the inaccurate error message could be misleading for issue analysis.

This trivial PR refine the error message by simply print "Can not get stack trace from the compressed hprof file", the underlying exception from GZIPInputStream() or HprofReader() would give accurate error info.

Hi David,
Thanks for review and comments.

The existing message is a generic message covering the general operation of the whole try block. It seems far more appropriate than your new message, which only seems to apply to the final step. ???

IMO, the whole method named getStack() is used to get the stack trace for the hprof file. And the try block could throw exceptions by serveral reasons as decribed, so IMHO printing "Can not decompress the compressed hprof file" is not accurate. The proposed change is to print a generic error message that the getStack() fail when processing compressed hprof. And the extra 'e' argument in the changed line would give detailed reason.

So may be change error message to "Can not process the compressed hprof file" is better? what do you think?

Yes that seems a good middle ground.

Alternatively, but more effort, have different try/catch blocks for the
two sections of the code. The first to decompress and write out; the
second to extract the stack traces. It isn't obvious that exceptions are
possible from both parts.

Thanks,
David

1 similar comment
@mlbridge
Copy link

@mlbridge mlbridge bot commented Jul 8, 2021

Mailing list message from David Holmes on hotspot-runtime-dev:

Hi Lin,

On 6/07/2021 1:33 pm, Lin Zang wrote:

On Tue, 6 Jul 2021 02:55:37 GMT, Lin Zang <lzang at openjdk.org> wrote:

The current implementation of hprof Reader for testing always prompts "Can not decompress the compressed hprof file" when there is error for testing gzipped heap dump. This is inaccurate if the gzipped file was decompressed successfully but the hprof file format is incorrect. So the inaccurate error message could be misleading for issue analysis.

This trivial PR refine the error message by simply print "Can not get stack trace from the compressed hprof file", the underlying exception from GZIPInputStream() or HprofReader() would give accurate error info.

Hi David,
Thanks for review and comments.

The existing message is a generic message covering the general operation of the whole try block. It seems far more appropriate than your new message, which only seems to apply to the final step. ???

IMO, the whole method named getStack() is used to get the stack trace for the hprof file. And the try block could throw exceptions by serveral reasons as decribed, so IMHO printing "Can not decompress the compressed hprof file" is not accurate. The proposed change is to print a generic error message that the getStack() fail when processing compressed hprof. And the extra 'e' argument in the changed line would give detailed reason.

So may be change error message to "Can not process the compressed hprof file" is better? what do you think?

Yes that seems a good middle ground.

Alternatively, but more effort, have different try/catch blocks for the
two sections of the code. The first to decompress and write out; the
second to extract the stack traces. It isn't obvious that exceptions are
possible from both parts.

Thanks,
David

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-runtime integrated serviceability
3 participants