On Fri, 24 Nov 2023 11:55:15 GMT, Alan Bateman <[email protected]> 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
