On Mon, 4 Mar 2024 19:35:48 GMT, Chris Plummer <[email protected]> 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
