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

[Jaikiran Pai]

On Mon, 4 Mar 2024 19:35:48 GMT, Chris Plummer <cjplummer at openjdk.org> wrote:

>> 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.

Hello Chris,
> 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 agree. 

> 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.

What Leonid suggests looks fine to me and keeps things simple.

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

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