On Fri, 24 Nov 2023 13:00:33 GMT, Jaikiran Pai <j...@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
