Reviewer and committer request for 7198496

[David Holmes]

On 4/10/2012 5:43 PM, Paul Sandoz wrote:
>
> On Oct 4, 2012, at 12:33 AM, David Holmes<david.holmes at oracle.com>  wrote:
>
>> On 3/10/2012 11:37 PM, Paul Sandoz wrote:
>>> On Oct 3, 2012, at 3:17 PM, David Holmes<david.holmes at oracle.com>   wrote:
>>>> On 3/10/2012 10:50 PM, Paul Sandoz wrote:
>>>>> For the benefit of others; for some context see this recent thread:
>>>>>
>>>>> http://markmail.org/search/?q=openjdk#query:openjdk%20list%3Anet.java.openjdk.core-libs-dev%20order%3Adate-backward+page:2+mid:inod424lqbey5fms+state:results
>>>>>
>>>>> Basically ServiceLoader is treating a null CL parameter as the system CL for loading the META-INF/services resources file and as the bootstrap CL for loading the classes of class names declared in those resource files.
>>>>>
>>>>> Which also means that calls to:
>>>>>
>>>>>    ServiceLoader.load(serviceInterface)
>>>>>    ServiceLoader.load(serviceInterface, Thread.currentThread().getContextClassLoader());
>>>>>
>>>>> Will behave oddly if the TCCL is null, from the JavaDoc of Thread.getContextClassLoader():
>>>>>
>>>>>    http://docs.oracle.com/javase/7/docs/api/java/lang/Thread.html#getContextClassLoader()
>>>>>    Returns:
>>>>>    the context ClassLoader for this Thread, or null indicating the system class loader (or, failing that, the bootstrap class loader)
>>>>
>>>> The @return doc for getContextClassLoader is wrong, or at best mis-leading. A null return does NOT indicate the system class loader (which is never null) - it simply means there is no CCL for that thread. If anything null would represent the bootstrap loader.
>>>
>>>>
>>>
>>> In the JDK i see code such as:
>>>
>>>                  Class c = Class.forName(className, true,Thread.currentThread().
>>>                                          getContextClassLoader());
>>>
>>> and:
>>>
>>>          ClassLoader contextClassLoader =
>>>              Thread.currentThread().getContextClassLoader();
>>>          if (contextClassLoader == null) {
>>>              contextClassLoader = ClassLoader.getSystemClassLoader();
>>>          }
>>>
>>> The only way i can interpret that TCCL JavaDoc sanely is to assume "null" is overloaded to mean the caller should try to use system or bootstrap CL if possible, otherwise failing that the bootstrap CL.
>>
>> The TCCL doc for @return is simply confusing.
>
>> The method will return the TCCL or it will return null. If it returns null it means the Thread has no CCL set. So what does a caller then do? They have a few choices:
>> a) use their own ClassLoader
>> b) use the system ClassLoader
>> c) use the the bootstrap loader
>>
>> Your first example chooses (c) because the null arg to forName will indicate to use the bootstrap loader. Your second example chooses (b). This is not an arbitrary choice as it depends on the API - the three arg version of forName treats null as the bootstrap loader, but if you simply did getContextClassLoader().loadClass(...) then you would get NPE.
>>
>
>> The point is that returning null in itself has no direct significance.
>
> I am not sure i agree. The TCCL can be set to null and that has significance as per the JavaDoc:
>
>       * @param  cl
>       *         the context ClassLoader for this Thread, or null  indicating the
>       *         system class loader (or, failing that, the bootstrap class loader)
>       *
>       * @throws  SecurityException
>       *          if the current thread cannot set the context ClassLoader
>       *
>       * @since 1.2
>       */
>      public void setContextClassLoader(ClassLoader cl) {

Sorry but that is just the dual incorrect statement of what is in 
getContextClassLoader. It is not a meaningful description, in fact it is 
incorrect because null only ever means the bootstrap loader, never the 
system class loader - and the null can't mean two different things. And 
what is "or failing that" supposed to mean?

These parts of the javadoc for get/setContextClassLoader are simply wrong.

David
-----

> Paul.
>
>> The issue here is how ServiceLoader should attempt to load something if there is no CCL set for the thread.
>>