Re: RFR: 8320687: sun.jvmstat.monitor.MonitoredHost.getMonitoredHost() throws unexpected exceptions when invoked concurrently [v4]
On Fri, 24 Nov 2023 13:00:33 GMT, Jaikiran Pai 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. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > Alan's suggestion - don't share ServiceLoader Hello Chris, I misunderstood the 2 Reviewer rule for serviceablitiy area then. I thought it was 2 Reviewers (capital R). I'll go ahead and integrate this shortly. Thank you all for the help and reviews. - PR Comment: https://git.openjdk.org/jdk/pull/16805#issuecomment-1839816547
Re: RFR: 8320687: sun.jvmstat.monitor.MonitoredHost.getMonitoredHost() throws unexpected exceptions when invoked concurrently [v4]
On Fri, 1 Dec 2023 07:59:49 GMT, Jaikiran Pai wrote: > I'm looking for one more Reviewer, please. You have 2 reviewers already. - PR Comment: https://git.openjdk.org/jdk/pull/16805#issuecomment-1839539742
Re: RFR: 8320687: sun.jvmstat.monitor.MonitoredHost.getMonitoredHost() throws unexpected exceptions when invoked concurrently [v4]
On Sat, 25 Nov 2023 06:23:25 GMT, Jaikiran Pai wrote: > Since serviceability area requires 2 Reviewers, could I get one more > Reviewer to review this please? I'm looking for one more Reviewer, please. - PR Comment: https://git.openjdk.org/jdk/pull/16805#issuecomment-1835642089
Re: RFR: 8320687: sun.jvmstat.monitor.MonitoredHost.getMonitoredHost() throws unexpected exceptions when invoked concurrently [v4]
On Fri, 24 Nov 2023 13:00:33 GMT, Jaikiran Pai 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. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > Alan's suggestion - don't share ServiceLoader Thank you Kevin and Alan for the reviews. tier1, tier2, tier3 and svc_tools all passed with this latest state of the PR. Since serviceability area requires 2 Reviewers, could I get one more Reviewer to review this please? - PR Comment: https://git.openjdk.org/jdk/pull/16805#issuecomment-1826229504
Re: RFR: 8320687: sun.jvmstat.monitor.MonitoredHost.getMonitoredHost() throws unexpected exceptions when invoked concurrently [v4]
On Fri, 24 Nov 2023 14:57:08 GMT, Jaikiran Pai wrote: >> src/jdk.internal.jvmstat/share/classes/sun/jvmstat/monitor/MonitoredHost.java >> line 170: >> >>> 168: break; >>> 169: } >>> 170: } >> >> Alternatively, you can do it more succulently with a stream: >> >> >> MonitoredHost mh = ServiceLoader.load(MonitoredHostService.class, >> >> MonitoredHostService.class.getClassLoader()) >> .stream() >> .map(ServiceLoader.Provider::get) >> .filter(mhs -> >> mhs.getScheme().equals(hostId.getScheme())) >> .map(mhs -> mhs.getMonitoredHost(hostId)) >> .findAny() >> .orElse(null); > >> .map(mhs -> mhs.getMonitoredHost(hostId)) > > That was a reasoanable suggestion and I gave it a try, but > `mhs.getMonitoredHost(hostId)` throws a checked exception `MonitorException` > so using this `Stream` based approach then would need additional exception > catching, wrapping and throwing, then catching this wrapped exception, > checking the cause and rethrowing. That then defeats the purpose of using the > `Stream` approach. > > I see that you have approved the PR, so I think you are OK with the current > non-stream approach. I'll now trigger the tier tests and if all comes back > fine, I'll go ahead and integrate this in a day or two. > > Kevin, does this latest form of the PR look OK to you too? All good! - PR Review Comment: https://git.openjdk.org/jdk/pull/16805#discussion_r1404446346
Re: RFR: 8320687: sun.jvmstat.monitor.MonitoredHost.getMonitoredHost() throws unexpected exceptions when invoked concurrently [v4]
On Fri, 24 Nov 2023 14:31:56 GMT, Alan Bateman wrote: > .map(mhs -> mhs.getMonitoredHost(hostId)) That was a reasoanable suggestion and I gave it a try, but `mhs.getMonitoredHost(hostId)` throws a checked exception `MonitorException` so using this `Stream` based approach then would need additional exception catching, wrapping and throwing, then catching this wrapped exception, checking the cause and rethrowing. That then defeats the purpose of using the `Stream` approach. I see that you have approved the PR, so I think you are OK with the current non-stream approach. I'll now trigger the tier tests and if all comes back fine, I'll go ahead and integrate this in a day or two. Kevin, does this latest form of the PR look OK to you too? - PR Review Comment: https://git.openjdk.org/jdk/pull/16805#discussion_r1404440378
Re: RFR: 8320687: sun.jvmstat.monitor.MonitoredHost.getMonitoredHost() throws unexpected exceptions when invoked concurrently [v4]
On Fri, 24 Nov 2023 13:00:33 GMT, Jaikiran Pai 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. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > Alan's suggestion - don't share ServiceLoader Marked as reviewed by alanb (Reviewer). src/jdk.internal.jvmstat/share/classes/sun/jvmstat/monitor/MonitoredHost.java line 170: > 168: break; > 169: } > 170: } Alternatively, you can do it more succulently with a stream: MonitoredHost mh = ServiceLoader.load(MonitoredHostService.class, MonitoredHostService.class.getClassLoader()) .stream() .map(ServiceLoader.Provider::get) .filter(mhs -> mhs.getScheme().equals(hostId.getScheme())) .map(mhs -> mhs.getMonitoredHost(hostId)) .findAny() .orElse(null); - PR Review: https://git.openjdk.org/jdk/pull/16805#pullrequestreview-1748045810 PR Review Comment: https://git.openjdk.org/jdk/pull/16805#discussion_r1404414161
Re: RFR: 8320687: sun.jvmstat.monitor.MonitoredHost.getMonitoredHost() throws unexpected exceptions when invoked concurrently [v4]
On Fri, 24 Nov 2023 11:55:15 GMT, Alan Bateman wrote:
>>> > 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`.
>>>
>>> Okay, I guess part of me wonders why this field is needed in the first
>>> place. Why can't getMonitoredHost just create a ServiceLoader instead
>>> instead of trying to share between threads?
>>
>>
>> It can and that works too. That was one of the alternatives I had initially
>> tried. I explain in this previous comment
>> https://github.com/openjdk/jdk/pull/16805#issuecomment-1825194163 the reason
>> why I thought sharing the ServiceLoader might be better. Do you think I
>> should just use the local ServiceLoader approach instead?
>
>> It can and that works too. That was one of the alternatives I had initially
>> tried. I explain in this previous comment [#16805
>> (comment)](https://github.com/openjdk/jdk/pull/16805#issuecomment-1825194163)
>> the reason why I thought sharing the ServiceLoader might be better. Do you
>> think I should just use the local ServiceLoader approach instead?
>
> I assume almost all usages just fetch the iterator just once, in which case
> the provider cache doesn't help. So yes, I think I would be tempted to just
> remove the field and make this a lot simpler.
Done. I've updated the PR to just use a local ServiceLoader instead of a shared
one.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16805#discussion_r1404327944
Re: RFR: 8320687: sun.jvmstat.monitor.MonitoredHost.getMonitoredHost() throws unexpected exceptions when invoked concurrently [v4]
> 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. Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision: Alan's suggestion - don't share ServiceLoader - Changes: - all: https://git.openjdk.org/jdk/pull/16805/files - new: https://git.openjdk.org/jdk/pull/16805/files/669e5e5e..6e7bf7fb Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16805=03 - incr: https://webrevs.openjdk.org/?repo=jdk=16805=02-03 Stats: 45 lines in 1 file changed: 16 ins; 20 del; 9 mod Patch: https://git.openjdk.org/jdk/pull/16805.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16805/head:pull/16805 PR: https://git.openjdk.org/jdk/pull/16805
