On Fri, 24 Nov 2023 06:06:16 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. Good sleuthing. src/jdk.internal.jvmstat/share/classes/sun/jvmstat/monitor/MonitoredHost.java line 142: > 140: // not thread safe, access MUST be guarded through synchronization > on "monitoredHosts" > 141: // field > 142: private static final ServiceLoader<MonitoredHostService> > monitoredHostServiceLoader = Can you put `assert Threads.holdLock(monitoredHosts)` at the top of the method, that will check that the monitor is held? Probably should fix the comment too, a bit strange to have a /* .. */ and // comment on the same method, looks like the style used in this code is /* .. */. test/jdk/sun/jvmstat/monitor/MonitoredVm/GetMonitoredHost.java line 43: > 41: * @run main/othervm GetMonitoredHost > 42: */ > 43: public class GetMonitoredHost { This is more of a regression test for the concurrent case rather than a unit test, so I think rename to Concurrent GetMonitoredHost to make it clearer what the test is far. ------------- PR Review: https://git.openjdk.org/jdk/pull/16805#pullrequestreview-1747367518 PR Review Comment: https://git.openjdk.org/jdk/pull/16805#discussion_r1403978916 PR Review Comment: https://git.openjdk.org/jdk/pull/16805#discussion_r1403979304
