On Tue, 5 Aug 2025 21:45:40 GMT, Archie Cobbs <[email protected]> wrote:
>> I'm a bit dubious about the code above because it does not appear to fully
>> handle the InterruptedException.
>> If it is really shutting down then its just complicit in leaving the
>> application is an unpredictable and probably unrecoverable state.
>>
>> Propagating the checked exception reduces the usability of close() to the
>> point that I would remove the waitFor.
>> It annoying enough already that `waitFor` throws InterruptedException and
>> you have to write more code and maybe a loop to handle that interruption and
>> what it means to the caller.
>> I would leave up to the caller to handle that case.
>> When used with try-with-resources, the exception handling/re-throwing is
>> complicated enough.
>
> You make some good points. Still, I think swallowing `InterruptException`'s
> in library code is generally considered to be wrong. According to _Java
> Concurrency in Practice_ ยง7.1.3:
>
>> If you don't want to or cannot propagate `InterruptedException`... you need
>> to find another way to preserve the interruption request. The standard way
>> to do this is to restore the interrupted status by calling interrupt again.
>> What you should not do is swallow the `InterruptedException` by catching it
>> and doing nothing in the catch block, unless your code is actually
>> implementing the interruption policy for a thread.
Can we simply restore the interrupt after `destroy()`?
} catch (InterruptedException _) {
try { destroy(); }
finally { Thread.currentThread.interrupt(); } // Restore the interrupt
}
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/26649#discussion_r2256323353
