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
