[jdk17u-dev] RFR: 8320687: sun.jvmstat.monitor.MonitoredHost.getMonitoredHost() throws unexpected exceptions when invoked concurrently [v3]

[Paul Hohensee]

On Fri, 23 May 2025 14:23:19 GMT, Volker Simonis <simonis at openjdk.org> wrote:

>> Paul Hohensee has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   8320687: Revert trigger
>
> Looks good.

Thanks for your review, @simonis.

> test/jdk/sun/jvmstat/monitor/MonitoredVm/ConcurrentGetMonitoredHost.java line 60:
> 
>> 58:         System.out.println("Submitting " + numTasks + " concurrent tasks to" +
>> 59:                 " get MonitoredHost for " + vmid);
>> 60:         try {
> 
> As you've already mentioned, the original implementation uses try-with-resources to close the `ExecutorService` but `ExecutorService` only implements `AutoClosable` since JDK 19. Alternatively, we could put the new `ExecutorService`'s `close()` handling into a `finallly` block:
> 
>         boolean terminated = isTerminated();
>         if (!terminated) {
>             shutdown();
>             boolean interrupted = false;
>             while (!terminated) {
>                 try {
>                     terminated = awaitTermination(1L, TimeUnit.DAYS);
>                 } catch (InterruptedException e) {
>                     if (!interrupted) {
>                         shutdownNow();
>                         interrupted = true;
>                     }
>                 }
>             }
>             if (interrupted) {
>                 Thread.currentThread().interrupt();
>             }
>         }
> 
> But that seems to be overkill, taking into account that in the body of the `try/catch` we already wait for the completion of all `ExecutorService` tasks. So I leave it up to you if you want the additional safe guard.

I agree. The test exits immediately in any case, so cleanup doesn't matter.

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

PR Comment: https://git.openjdk.org/jdk17u-dev/pull/3593#issuecomment-2905093180
PR Review Comment: https://git.openjdk.org/jdk17u-dev/pull/3593#discussion_r2105017246