RFR: 8223260: NamingManager should cache InitialContextFactory

[Peter Levart]

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