On Sat, 2 Mar 2024 06:21:09 GMT, Jaikiran Pai <j...@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
