RFR: 8223260: NamingManager should cache InitialContextFactory

[Seán Coffey]

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>>

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
>