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

Reply via email to