On Fri, 24 Nov 2023 06:23:03 GMT, Alan Bateman <al...@openjdk.org> wrote:
>> Jaikiran Pai has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - Alan's review suggestion - rename GetMonitoredHost to >> ConcurrentGetMonitoredHost >> - fix code comment > > 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 /* .. */. Hello Alan, I've updated the PR to fix the code comment section to use the existing `/* */` style. > Can you put assert Threads.holdLock(monitoredHosts) at the top of the method, > that will check that the monitor is held? Right now, the sole usage of the `monitoredHostServiceLoader` instance is within a `synchronized (monitoredHosts) {...}` block within a method. So it wouldn't require this `assert`. > 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. Done. I've now updated the PR to rename this new test to ConcurrentGetMonitoredHost. Test continues to pass. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/16805#discussion_r1403999159 PR Review Comment: https://git.openjdk.org/jdk/pull/16805#discussion_r1403997773