On 2/5/20 12:53 PM, Peter Levart wrote:
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?
For example. If some hypothetical InitialContextFactory implementation
keeps a reference to an object which's class was loaded by some child
ClassLoader of the ClassLoader that loaded the InitialContextFactory
implementation class or some unrelated ClassLoader then it is the
responsibility of such implementation to wrap such object into a
XxxReference or else it is keeping the child/unrelated ClassLoader
reachable and unavailable for GC for as long as the code uses
InitialContextFactory instance regardless of whether such instance is
also cached or not. So it's the policy of the service to decide what it
keeps strongly and what weakly reachable, not the user of the service...
What ClassLoaderValue gives is direct reachability from the ClassLoader
instance to the cached key/value. Such ClassLoader becomes eligible for
GC as soon as program relinquishes all references to objects backed by
classes loaded by this ClassLoader. The ClassLoaderValue instance(s)
itself do not keep a strong reference to either the ClassLoader or the
keys/values cached for such ClassLoader. So in a way ClassLoaderValue is
similar to ClassValue, just Class is replaced with ClassLoader.
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
