On Fri, 24 Nov 2023 06:23:03 GMT, Alan Bateman <[email protected]> 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
