RFR: 8320687: sun.jvmstat.monitor.MonitoredHost.getMonitoredHost() throws unexpected exceptions when invoked concurrently [v4]

[Alan Bateman]

On Fri, 24 Nov 2023 13:00:33 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

>> Can I please get a review of this change which proposes to fix the issue noted in https://bugs.openjdk.org/browse/JDK-8320687?
>> 
>> As noted in the issue, the `sun.jvmstat.monitor.MonitoredHost.getMonitoredHost()` uses a shared instance of `java.util.ServiceLoader` to load `MonitoredHostService` services. The `ServiceLoader` class javadoc explicitly notes that it isn't thread safe. The issue at hand is caused to due using an instance of `ServiceLoader` concurrently by multiple threads.
>> 
>> The fix proposes to guard the usage of the shared `ServiceLoader` instance through the `monitoredHosts` object monitor. We already use that monitor when dealing with the internal cache which is populated after loading the relevant `MonitoredHostService`(s).
>> 
>> A new jtreg test has been introduced which always reproduces the issue without the source changes and passes with this fix.
>> 
>> tier1, tier2, tier3 and svc_tools tests have been run with this change and all passed.
>
> Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Alan's suggestion - don't share ServiceLoader

Marked as reviewed by alanb (Reviewer).

src/jdk.internal.jvmstat/share/classes/sun/jvmstat/monitor/MonitoredHost.java line 170:

> 168:                 break;
> 169:             }
> 170:         }

Alternatively, you can do it more succulently with a stream:


MonitoredHost mh = ServiceLoader.load(MonitoredHostService.class,
                                                  MonitoredHostService.class.getClassLoader())
                    .stream()
                    .map(ServiceLoader.Provider::get)
                    .filter(mhs -> mhs.getScheme().equals(hostId.getScheme()))
                    .map(mhs -> mhs.getMonitoredHost(hostId))
                    .findAny()
                    .orElse(null);

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

PR Review: https://git.openjdk.org/jdk/pull/16805#pullrequestreview-1748045810
PR Review Comment: https://git.openjdk.org/jdk/pull/16805#discussion_r1404414161