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

[Alan Bateman]

On Fri, 24 Nov 2023 06:06:16 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.

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