RFR: 8223260: NamingManager should cache InitialContextFactory

[Seán Coffey]

Thanks for the review Peter. All good points! My latest patch contains 
adjustments based on feedback from you and others to date.

* Incorporate use of ClassLoaderValue -
  -- I'd appreciate feedback on whether I'm using it correctly.
* Use of ServiceLoader.stream()
* adjusted test for both the ServiceLoader and legacy classpath load 
approach
* insert use of sleep which testing GC.

http://cr.openjdk.java.net/~coffeys/webrev.8223260.v2/webrev/

regards,
Sean.

On 30/01/2020 07:59, Peter Levart wrote:
> Hi Seán,
>
> WeakHashMap is not safe to be called concurrently. Even get() method, 
> although it may seem read-only, can modify internal state (expunging 
> stale Weak entries), so holding a READ lock while accessing 
> WeakHashMap it is wrong.
>
> getInitialContext() static method is called with an env Hashtable 
> which means it can be called with different keys/values for the same 
> TCCL. So caching of InitialContextFactory is just performed for the 
> 1st call for a particular TCCL. Subsequent calls for the same TCCL and 
> different class names are not cached. Is this the behavior you are 
> pursuing? You could cache the factory using (TCCL, class name) as a 
> compound key.
>
> Also, by caching in a WeakHashMap<ClassLoader, InitialContextFactory>, 
> you make a strong reference to InitialContextFactory from class loader 
> of the NamingManager.class and such InitialContextFactory may 
> indirectly reference the ClassLoader key of the same entry, so it will 
> never go away because NamingManager class is never going away. You 
> should at least use a WeakHashMap<ClassLoader, 
> WeakReference<InitialContextFactory>> for that.
>
> Shameless plug: there is a JDK internal class 
> jdk.internal.loader.ClassLoaderValue which you might be able to use 
> for caching if a part of your key is a ClassLoader. From the javadoc:
>
>  * ClassLoaderValue allows associating a
>  * {@link #computeIfAbsent(ClassLoader, BiFunction) computed} non-null 
> value with
>  * a {@code (ClassLoader, keys...)} tuple. The associated value, as 
> well as the
>  * keys are strongly reachable from the associated ClassLoader so care 
> should be
>  * taken to use such keys and values that only reference types 
> resolvable from
>  * the associated ClassLoader. Failing that, ClassLoader leaks are 
> inevitable.
>
> So if you know that the InitialContextFactory instance is always 
> resolvable (by class name) from the ClassLoader you are using for the 
> caching key (the TCCL), then this utility might be just right for your 
> purpose.
>
> Regards, Peter
>
>
> On 1/29/20 6:22 PM, Seán Coffey wrote:
>> Thanks for the reviews. I found an issue with the new test also -
>>
>> it's loading the custom factory class via the non-serviceloader 
>> approach. I was hoping to exercise ServiceLoader here. I'll address 
>> this and the comments raised and revert with a new patch shortly.
>>
>> Regards,
>> Sean.
>>
>> On 29/01/20 16:27, Alan Bateman wrote:
>>> On 29/01/2020 15:55, Daniel Fuchs wrote:
>>>> Hi Seán,
>>>>
>>>>> http://cr.openjdk.java.net/~coffeys/webrev.8223260.v1/webrev/
>>>>> A WeakHashKey with the TCCL as the key should be okay here.
>>>>
>>>> If the TCCL is the key then there are good chances that the
>>>> concrete factory class is expected to be loaded by the TCCL.
>>>>
>>>> If that happens then the value will reference the key and
>>>> nothing will ever get garbage collected.
>>> I don't know how much JNDI is used much beyond LDAP these days but 
>>> you are right that a factory with a strong ref to the TCCL would 
>>> prevent it from being GC'ed.  The internal WeakPairMap might be 
>>> useful here.
>>>
>>> -Alan
>>
>