Paul,
On 4/10/2012 7:53 PM, Paul Sandoz wrote:
On Oct 4, 2012, at 11:34 AM, David Holmes<[email protected]> wrote:
These parts of the javadoc for get/setContextClassLoader are simply wrong.
Wrong or not because of such JavaDoc developers have been coding using TCCL with certain
expectations of "null" different to that of just meaning the bootstrap CL.
How would you propose to fix it?
I don't see anything that actually needs fixing (apart from the javadoc).
Anyone using getCCL has to expect null as a possibility and they are then free
to proceed however they like. The obvious ways to proceed are as I outlined
earlier:
- use current classloader
- use system loader
- use bootstrap loader
So you are proposing to widen the scope? since i interpret the current JavaDoc
to correspond to only the latter two options.
I think you are missing the point. If getCCL returns null it returns
null - end of story. What the caller of getCCL does after that is their
business, it has nothing to do with the spec for getCCL. As Chris said
getCCL can suggest that if null is returned then the caller might use
the system or bootloader but that is all it is - a suggestion.
The issue here is what ServiceLoader says. First load(Class,
ClassLoader) states:
* @param loader
* The class loader to be used to load provider-configuration files
* and provider classes, or <tt>null</tt> if the system class
* loader (or, failing that, the bootstrap class loader) is to be
* used
which has the same language as used for the CCL. However here it is
mostly well defined: if the passed in loader is null then use the system
loader. The "or failing that, the bootstrap class loader" seems
redundant - as the system loader (unless overridden I suppose) will
delegate to the bootstrap loader first, there would be no point in then
trying the bootstrap loader directly. But conservatively, if the system
loader had been overridden you might also try the bootloader just in
case. (But the implementation does not do that.)
Then load(Class) says:
/**
* Creates a new service loader for the given service type, using the
* current thread's {@linkplain java.lang.Thread#getContextClassLoader
* context class loader}.
This is fine. If the CCL is null then the semantics for
load(service,null) come into play.
So now we come to code in question:
private ServiceLoader(Class<S> svc, ClassLoader cl) {
service = Objects.requireNonNull(svc, "Service interface cannot be
null");
loader = cl;
reload();
}
No problem so far - loader can be null. The question is then how it is
used later ...
if (loader == null)
configs = ClassLoader.getSystemResources(fullName);
else
configs = loader.getResources(fullName);
So the question now is what does ClassLoader.getSystemResources do? It says:
"Finds all resources of the specified name from the search path used to
load classes. "
That sounds to me like it uses the system classloader, which in turn
would first delegate to the bootstrap loader. All good. But we also have:
try {
S p = service.cast(Class.forName(cn, true, loader)
.newInstance());
and there is the bug because if you pass null to forName it will use the
bootstrap loader, not the system loader. So it seems to me that a fix
here is to just change the above to:
Class.forName(cn, true, loader != null ? loader :
Classloader.getSystemClassLoader())
Instead you have opted to deal with a null loader at construction time:
loader = (cl == null) ? ClassLoader.getSystemClassLoader() : cl;
which has the same semantic effect, but leaves some "dead" code
internally as now loader can not be null.
David
Paul.
