Reviewer and committer request for 7198496

[David Holmes]

Paul,

On 4/10/2012 7:53 PM, Paul Sandoz wrote:
> On Oct 4, 2012, at 11:34 AM, David Holmes<david.holmes at oracle.com>  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.