On Sat, 2 Mar 2024 04:33:11 GMT, Chris Plummer <cjplum...@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
