RFR: 8223260: NamingManager should cache InitialContextFactory

[Peter Levart]

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
>