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

[Jaikiran Pai]

On Fri, 24 Nov 2023 06:23:03 GMT, Alan Bateman <alanb at 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