Peter,
thanks again for your review. comments inline..
On 31/01/2020 17:16, Peter Levart wrote:
Hi Seán,
On 1/31/20 2:16 PM, Seán Coffey wrote:
Thanks for the review Peter. All good points! My latest patch
contains adjustments based on feedback from you and others to date.
* Incorporate use of ClassLoaderValue -
-- I'd appreciate feedback on whether I'm using it correctly.
Yes, you used it correctly. You are very verbose about using it, but
that's just style. I, for example, would re-write this:
712 ClassLoaderValue<InitialContextFactory>.Sub<String> key =
FACTORIES_CACHE.sub(className);
713 try {
714 factory = key.computeIfAbsent(loader, (ld, ky) -> {
715 String cn = ky.key();
716 InitialContextFactory fac = getFactory(cn);
717 return fac;
718 });
719 } catch (UndeclaredThrowableException e) {
720 if (e.getUndeclaredThrowable() instanceof
NoInitialContextException) {
721 throw (NoInitialContextException)
e.getUndeclaredThrowable();
722 }
723 }
... into this:
var key = FACTORIES_CACHE.sub(className);
try {
factory = key.computeIfAbsent(loader, (ld, ky) ->
getFactory(ky.key()));
} catch (UndeclaredThrowableException e) {
if (e.getUndeclaredThrowable() instanceof
NoInitialContextException) {
throw (NoInitialContextException)
e.getUndeclaredThrowable();
} else {
throw e;
}
}
Yes - looks much neater. I've edited the patch to that effect.
Notice also that I added:
} else {
throw e;
}
You have two options. Either UndeclaredThrowableException is possible
only when you wrap NoInitialContextException with it in getFactory()
in which case you could simply do unconditional:
} catch (UndeclaredThrowableException e) {
throw (NoInitialContextException)
e.getUndeclaredThrowable();
}
...or UndeclaredThrowableException is possible also to be thrown from
code called by getFactory() (in theory, it is). In this case you would
want to re-throw it here instead of swallowing it.
Yes - I was wondering if I should be concerned about other call sites
that might throw UndeclaredThrowableException. You're right, best to be
on the safe side and re-throw. Code edited.
* Use of ServiceLoader.stream()
737 factory = loader
738 .stream()
739 .map(ServiceLoader.Provider::get)
740 .filter(f ->
f.getClass().getName().equals(className))
741 .findFirst()
742 .orElse(null);
Here you instantiate InitialContextFactory instances until you get one
with implementation class with correct className. But Provider.type()
should already return the type of the service without instantiating
it. So you could write the following instead:
factory = loader
.stream()
.filter(p -> p.type().getName().equals(className))
.findFirst()
.map(ServiceLoader.Provider::get)
.orElse(null);
That makes sense. It actually explains a test failure I was seeing
earlier today while trying to expand test coverage for this issue. Off
mail thread, Daniel Fuchs suggested I use a more concrete URLClassLoader
example. I've introduced extra testing to test for multiple
Context.INITIAL_CONTEXT_FACTORY values. I was getting unexpected
initialization values since the stream function was instantiating
DummyContextFactory for filter function (when in fact
DummyContextFactory2 ended up being the correct Factory) . Thanks! I've
adopted this change.
This should instantiate just the service that is found and not any
other. ServiceLoader.Provider.type() is specified to return:
* Returns the provider type. There is no guarantee that this
type is
* accessible or that it has a public no-args constructor. The
{@link
* #get() get()} method should be used to obtain the provider
instance.
*
* <p> When a module declares that the provider class is
created by a
* provider factory then this method returns the return type
of its
* public static "{@code provider()}" method.
So in theory this method could return a super-type of the service
implementation class. But one could argue that the name of the service
provider type is the one we should be searching for, not the
implementation class of the service. Perhaps the service declares a
single provider type and then at instantiation time it dynamically
chooses an implementation class depending on the environment
(architecture perhaps). It would be interesting to see whether
provider types in real service implementations differ from service
implementation classes or are they usually the same.
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.
current webrev:
https://cr.openjdk.java.net/~coffeys/webrev.8223260.v3/webrev/
regards,
Sean.
Regards, Peter
* adjusted test for both the ServiceLoader and legacy classpath load
approach
* insert use of sleep which testing GC.
http://cr.openjdk.java.net/~coffeys/webrev.8223260.v2/webrev/
regards,
Sean.
On 30/01/2020 07:59, Peter Levart wrote:
Hi Seán,
WeakHashMap is not safe to be called concurrently. Even get()
method, although it may seem read-only, can modify internal state
(expunging stale Weak entries), so holding a READ lock while
accessing WeakHashMap it is wrong.
getInitialContext() static method is called with an env Hashtable
which means it can be called with different keys/values for the same
TCCL. So caching of InitialContextFactory is just performed for the
1st call for a particular TCCL. Subsequent calls for the same TCCL
and different class names are not cached. Is this the behavior you
are pursuing? You could cache the factory using (TCCL, class name)
as a compound key.
Also, by caching in a WeakHashMap<ClassLoader,
InitialContextFactory>, you make a strong reference to
InitialContextFactory from class loader of the NamingManager.class
and such InitialContextFactory may indirectly reference the
ClassLoader key of the same entry, so it will never go away because
NamingManager class is never going away. You should at least use a
WeakHashMap<ClassLoader, WeakReference<InitialContextFactory>> for
that.
Shameless plug: there is a JDK internal class
jdk.internal.loader.ClassLoaderValue which you might be able to use
for caching if a part of your key is a ClassLoader. From the javadoc:
* ClassLoaderValue allows associating a
* {@link #computeIfAbsent(ClassLoader, BiFunction) computed}
non-null value with
* a {@code (ClassLoader, keys...)} tuple. The associated value, as
well as the
* keys are strongly reachable from the associated ClassLoader so
care should be
* taken to use such keys and values that only reference types
resolvable from
* the associated ClassLoader. Failing that, ClassLoader leaks are
inevitable.
So if you know that the InitialContextFactory instance is always
resolvable (by class name) from the ClassLoader you are using for
the caching key (the TCCL), then this utility might be just right
for your purpose.
Regards, Peter
On 1/29/20 6:22 PM, Seán Coffey wrote:
Thanks for the reviews. I found an issue with the new test also -
it's loading the custom factory class via the non-serviceloader
approach. I was hoping to exercise ServiceLoader here. I'll address
this and the comments raised and revert with a new patch shortly.
Regards,
Sean.
On 29/01/20 16:27, Alan Bateman wrote:
On 29/01/2020 15:55, Daniel Fuchs wrote:
Hi Seán,
http://cr.openjdk.java.net/~coffeys/webrev.8223260.v1/webrev/
A WeakHashKey with the TCCL as the key should be okay here.
If the TCCL is the key then there are good chances that the
concrete factory class is expected to be loaded by the TCCL.
If that happens then the value will reference the key and
nothing will ever get garbage collected.
I don't know how much JNDI is used much beyond LDAP these days but
you are right that a factory with a strong ref to the TCCL would
prevent it from being GC'ed. The internal WeakPairMap might be
useful here.
-Alan
