RFR: 8269909: getStack method in hprof.parser.Reader should use try-with-resource [v4]

[Lin Zang]

On Thu, 15 Jul 2021 17:22:31 GMT, Serguei Spitsyn <sspitsyn at openjdk.org> wrote:

>> Lin Zang has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains five additional commits since the last revision:
>> 
>>  - fix indentation issue
>>  - Merge branch 'master' into try
>>  - revise code to handle the closing of embeded streams
>>  - Merge branch 'master' into try
>>  - 8269909: getStack method in hprof.parser.Reader should use try-with-resource
>
> Hi Lin,
> These local names with extra numbers look strange.
> You introduced these numbers in order to fix naming conflicts.
> You also can avoid these conflicts by refactoring the code.
> Some of these fragments can be refactored to become a separate methods.
> I do not want to push hard on you with this but it is just something to consider to simplify the code and avoid such naming problems.
> Thanks,
> Serguei

Dear Serguei(@sspitsyn ),

> Some of these fragments can be refactored to become a separate methods.

I have tried to extract the common codes to a seperate method : 


HprofReader getHprofReader() {

    try (FileInputStream outFis = new FileInputStream(out);
       BufferedInputStream outBis = new BufferedInputStream(outFis);
       PositionDataInputStream pdin = new PositionDataInputStream(outBis)) {
       i = pdin.readInt();
       if (i == HprofReader.MAGIC_NUMBER) {
       HprofReader r
                = new HprofReader(deCompressedFile, pdin, dumpNumber,
                                                  true, debugLevel);
       return r;
}

and then call:


HprofReader r = getHprofReader();
r.read(); // or r.printStackTraces();

But it doesn't work because the streams are closed when method `getHprofReader` returns, and the `r.read()` or `r.printStackTraces()` still need to read data from the stream, which throws exception.

Moreover, I also found that the method `getStack()` and `readFile()` have almost the same logic. So maybe there is a chance that they could be combined together, or one the them could be omit if only used for testing purpose (may also need to adjust the related heap dump test cases). And IMHO the `GzipRandomAccess` class may not even be necessary then.

However, I think it is not quite relate to this PR. So I just update the patch that only updated the variable name. and I suggest to use a new issue for code refecting in future. 

What do you think? 

Thanks!
Lin

-------------

PR: https://git.openjdk.java.net/jdk/pull/4717