[jdk17u-dev] RFR: 8320687: sun.jvmstat.monitor.MonitoredHost.getMonitoredHost() throws unexpected exceptions when invoked concurrently [v3]
Paul Hohensee
phh at openjdk.org
Fri May 23 16:49:56 UTC 2025
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
More information about the jdk-updates-dev
mailing list