On Fri, 24 Nov 2023 11:46:20 GMT, Jaikiran Pai <j...@openjdk.org> 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?
>
>> > 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.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/16805#discussion_r1404274170

Reply via email to