RFR: 8325532: serviceability/dcmd/compiler/PerfMapTest.java leaves created files in the /tmp dir.

[Chris Plummer]

On Sat, 2 Mar 2024 06:21:09 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

>> The one issue with the first variant is that when run() throws an exception, it will be printed twice, assuming deleteIfExists() does not throw an exception. Basically you either get the exception printed twice, or you get the exception printed once plus whatever exception deleteIfExists() throws also printed (which is ok).
>> 
>> I think your 2nd example will work, but it's kind of confusing because if you got into the finally block via an exception, you then might throw another exception, which is handled in the finally block, and then (implicitly) the original exception is rethrown as you exit the finally block. However, if there was no exception from run() but deleteIfExists() throws an exception, the exception is printed, but does not cause the test to fail. I don't think we want that.
>
> Hello Chris, I haven't followed this PR and this is just a drive by comment about the exception handling discussion. In some other areas of our JDK tests, for situations like this, we do the following:
> 
> 
> Throwable failure = null;
> try {
>     run(new JMXExecutor(), "Compiler.perfmap " + path.toString(), path);
> } catch (Throwable t) {
>     failure = t;
> } finally {
>     try {
>         Files.deleteIfExists(path);
>     } catch (IOException ioe) {
>         if (failure != null) {
>             // add the IOException as a suppressed exception to the original failure
>             // and rethrow the original
>             failure.addSuppressed(ioe);
>             throw failure;
>         }
>         // no failures in the run(), but deleting the file failed, rethrow this IOException
>         throw ioe;
>     }
> }
> 
> This way both the original test failure (if any) as well as the failure in the finally block (if any) are propagated and made available in the failure report.

Hi Jai. I think that solves all the functional requirements, but now we've turned two easily understood lines of code (calling run() and deleteIfExists()) into what you have above. I'm not so sure it is worth it. I refer back to Leonid's comment above on this topic:

> I thought about execution of Files.deleteIfExists(path) in the case of test fails, but decided that it is not so important. Also, test always might fail with crash when no finally block is executed. So I am fine with current way.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/17992#discussion_r1511697784