RFR: 8223260: NamingManager should cache InitialContextFactory
Seán Coffey
sean.coffey at oracle.com
Fri Jan 31 23:22:52 UTC 2020
Peter,
thanks again for your review. comments inline..
On 31/01/2020 17:16, Peter Levart wrote:
> Hi Seán,
>
> On 1/31/20 2:16 PM, Seán Coffey wrote:
>> 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.
>
> Yes, you used it correctly. You are very verbose about using it, but
> that's just style. I, for example, would re-write this:
>
> 712 ClassLoaderValue<InitialContextFactory>.Sub<String> key =
> FACTORIES_CACHE.sub(className);
> 713 try {
> 714 factory = key.computeIfAbsent(loader, (ld, ky) -> {
> 715 String cn = ky.key();
> 716 InitialContextFactory fac = getFactory(cn);
> 717 return fac;
> 718 });
> 719 } catch (UndeclaredThrowableException e) {
> 720 if (e.getUndeclaredThrowable() instanceof
> NoInitialContextException) {
> 721 throw (NoInitialContextException)
> e.getUndeclaredThrowable();
> 722 }
> 723 }
>
> ... into this:
>
> var key = FACTORIES_CACHE.sub(className);
> try {
> factory = key.computeIfAbsent(loader, (ld, ky) ->
> getFactory(ky.key()));
> } catch (UndeclaredThrowableException e) {
> if (e.getUndeclaredThrowable() instanceof
> NoInitialContextException) {
> throw (NoInitialContextException)
> e.getUndeclaredThrowable();
> } else {
> throw e;
> }
> }
Yes - looks much neater. I've edited the patch to that effect.
>
> Notice also that I added:
>
> } else {
> throw e;
> }
>
> You have two options. Either UndeclaredThrowableException is possible
> only when you wrap NoInitialContextException with it in getFactory()
> in which case you could simply do unconditional:
>
> } catch (UndeclaredThrowableException e) {
> throw (NoInitialContextException)
> e.getUndeclaredThrowable();
> }
>
> ...or UndeclaredThrowableException is possible also to be thrown from
> code called by getFactory() (in theory, it is). In this case you would
> want to re-throw it here instead of swallowing it.
Yes - I was wondering if I should be concerned about other call sites
that might throw UndeclaredThrowableException. You're right, best to be
on the safe side and re-throw. Code edited.
>
>> * Use of ServiceLoader.stream()
>
> 737 factory = loader
> 738 .stream()
> 739 .map(ServiceLoader.Provider::get)
> 740 .filter(f ->
> f.getClass().getName().equals(className))
> 741 .findFirst()
> 742 .orElse(null);
>
> Here you instantiate InitialContextFactory instances until you get one
> with implementation class with correct className. But Provider.type()
> should already return the type of the service without instantiating
> it. So you could write the following instead:
>
> factory = loader
> .stream()
> .filter(p -> p.type().getName().equals(className))
> .findFirst()
> .map(ServiceLoader.Provider::get)
> .orElse(null);
That makes sense. It actually explains a test failure I was seeing
earlier today while trying to expand test coverage for this issue. Off
mail thread, Daniel Fuchs suggested I use a more concrete URLClassLoader
example. I've introduced extra testing to test for multiple
Context.INITIAL_CONTEXT_FACTORY values. I was getting unexpected
initialization values since the stream function was instantiating
DummyContextFactory for filter function (when in fact
DummyContextFactory2 ended up being the correct Factory) . Thanks! I've
adopted this change.
>
> This should instantiate just the service that is found and not any
> other. ServiceLoader.Provider.type() is specified to return:
>
> * Returns the provider type. There is no guarantee that this
> type is
> * accessible or that it has a public no-args constructor. The
> {@link
> * #get() get()} method should be used to obtain the provider
> instance.
> *
> * <p> When a module declares that the provider class is
> created by a
> * provider factory then this method returns the return type
> of its
> * public static "{@code provider()}" method.
>
> So in theory this method could return a super-type of the service
> implementation class. But one could argue that the name of the service
> provider type is the one we should be searching for, not the
> implementation class of the service. Perhaps the service declares a
> single provider type and then at instantiation time it dynamically
> chooses an implementation class depending on the environment
> (architecture perhaps). It would be interesting to see whether
> provider types in real service implementations differ from service
> implementation classes or are they usually the same.
>
> The following is also possible:
>
> // 1st try finding a ServiceLoader.Provider with type() of
> correct name
> factory = loader
> .stream()
> .filter(p -> p.type().getName().equals(className))
> .findFirst()
> .map(ServiceLoader.Provider::get)
> .or( // if that doesn't yield any result, instantiate
> the services
> // one by one and search for implementation class
> of correct name
> () -> loader
> .stream()
> .map(ServiceLoader.Provider::get)
> .filter(f ->
> f.getClass().getName().equals(className))
> .findFirst()
> ).orElse(null);
>
> So what do you think?
ok - possible I guess but I think it's highly unlikely ? It looks like
alot of extra care for a case that shouldn't happen. I'll stick with
your earlier suggestion unless you or others object.
current webrev:
https://cr.openjdk.java.net/~coffeys/webrev.8223260.v3/webrev/
regards,
Sean.
>
> Regards, Peter
>
>> * 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
>>>>
>>>
>
More information about the core-libs-dev
mailing list