RFR: 8223260: NamingManager should cache InitialContextFactory

[Peter Levart]

On 2/5/20 9:31 AM, Seán Coffey wrote:
>
> Thanks again for the review Peter. There's an off-thread conversation 
> around whether the ClassLoaderValue should hold SoftReferences to the 
> Factory that's stored with the classloader. I think we're looking at a 
> possible leak otherwise.
> i.e. ClassLoaderValue<SoftReference<InitialContextFactory>>
>

Please, include me in the conversation. I would like to know whether 
this is really possible, because I think it is not. If the 
implementation class / provider type is resolved using thread's context 
class loader, then it is the responsibility of the service 
implementation to only reference such objects that are backed by classes 
that are also resolvable by the same class loader. If service 
implementation does not respect that, then class loader leaks are 
inevitable even if you don't cache the service implementation instance 
(in your case the InitialContextFactory). So I think there's no point in 
wrapping the InitialContextFactory with SoftReference. You only 
complicate things as you would have to account for situations where the 
SoftReference is cleared...
Anybody has a different view?

Peter

> I'm looking into that now.
>
> Also - I'm hoping to port this to JDK 11u also so I might spin the 
> specification changes into a different bug ID.
>
> regards,
> Sean.
>
> On 03/02/2020 09:05, Peter Levart wrote:
>> Hi Seán,
>>
>> On 2/1/20 12:22 AM, Seán Coffey wrote:
>>>> 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.
>>
>> For the 3 InitialContextFactory implementations in JDK 
>> (DnsContextFactory, RegistryContextFactory, LdapCtxFactory), none 
>> uses the provider() static method convention, so for them the 
>> Provider.type()s are actually the same as their implementation 
>> classes. Should other InitialContextFactory service providers use the 
>> provider() static method convention (they may do this only if they 
>> are provided as Java modules I think), the InitialContextFactory 
>> sub-type name searched for in the NamingManager.getInitialContext() 
>> method is the provider type name, and not the implementation class 
>> name of the InitialContextFactory. They are usually the same, but in 
>> case of provider() static method convention, they may or may not be. 
>> This is not a problem for JDK supplied implementations and I don't 
>> think for any other current implementation. But anyway, I think this 
>> distinction should be spelled out in the specification of the 
>> NamingManager.getInitialContext() method and this is an opportunity 
>> to add some text for that. For example:
>>
>> Index: src/java.naming/share/classes/javax/naming/spi/NamingManager.java
>> IDEA additional info:
>> Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
>> <+>UTF-8
>> ===================================================================
>> --- src/java.naming/share/classes/javax/naming/spi/NamingManager.java 
>> (revision 57904:0905868db490c87df463258166762797374e5a96)
>> +++ src/java.naming/share/classes/javax/naming/spi/NamingManager.java 
>> (revision 57904+:0905868db490+)
>> @@ -644,7 +660,9 @@
>>       *     <ul>
>>       *     <li>First, the {@linkplain java.util.ServiceLoader 
>> ServiceLoader}
>>       *         mechanism tries to locate an {@code 
>> InitialContextFactory}
>> -     *         provider using the current thread's context class 
>> loader</li>
>> +     *         provider for which the {@linkplain 
>> ServiceLoader.Provider#type()}
>> +     *         returns a type with name equal to {@code 
>> Context.INITIAL_CONTEXT_FACTORY}
>> +     *         environment property and using the current thread's 
>> context class loader</li>
>>       *     <li>Failing that, this implementation tries to locate a 
>> suitable
>>       *         {@code InitialContextFactory} using a built-in mechanism
>>       *         <br>
>> @@ -662,7 +680,7 @@
>>       * @return A non-null initial context.
>>       * @exception NoInitialContextException If the
>>       *          {@code Context.INITIAL_CONTEXT_FACTORY} property
>> -     *         is not found or names a nonexistent
>> +     *         is not found or names a nonexistent {@linkplain 
>> ServiceLoader.Provider#type()},
>>       *         class or a class that cannot be instantiated,
>>       *          or if the initial context could not be created for 
>> some other
>>       *          reason.
>>
>>
>>>
>>> current webrev: 
>>> https://cr.openjdk.java.net/~coffeys/webrev.8223260.v3/webrev/
>>>
>>
>> Otherwise, I think this webrev looks good now.
>>
>>> regards,
>>> Sean. 
>>
>> Regards, Peter
>>