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

[Jaikiran Pai]

On Sat, 2 Mar 2024 04:33:11 GMT, Chris Plummer <cjplummer at openjdk.org> wrote:

>> Two variants:
>> 
>>     public void runHelper(CommandExecutor executor, String cmd, Path path) {
>>        try {
>>            run(executor, cmd, path);
>>        } catch (Throwable t) {
>>             t.printStackTrace(System.out);
>>             throw t;
>>        } finally {
>>            Files.deleteIfExists(path);
>>        }
>>     }
>> 
>> or
>> 
>>     public void runHelper(CommandExecutor executor, String cmd, Path path) {
>>        try {
>>            run(executor, cmd, path);
>>        } finally {
>>            try {
>>               Files.deleteIfExists(path);
>>            } catch (Throwable t) {
>>               <print message about exception thrown from deleteIfExists>
>>               t.printStackTrace(System.out);
>>            }
>>        }
>>     }
>
> 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.

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

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