Reviewer and committer request for 7198496

[Peter Levart]

On 10/04/2012 03:37 PM, David Holmes wrote:
> On 4/10/2012 9:56 PM, Paul Sandoz wrote:
>>
>> On Oct 4, 2012, at 12:37 PM, David Holmes<david.holmes at oracle.com>  
>> wrote:
>>
>>> 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.
>>
>> I think we may be misunderstanding and/or talking over each other. 
>> IIUC you are saying what behaviour you would like and i am saying how 
>> i interpret the current behaviour.
>>
>> I interpret the current JavaDoc as normatively stating what the 
>> caller of getCCL *must* do if null a value is returned.
>
> And that is the problem. A spec for a method can not normatively say 
> what the caller of the method must do with the result.
>
>> A little tweak to the doc might help:
>>
>> From:
>>
>> "@return  the context ClassLoader for this Thread, or {@code null} 
>> indicating the system class loader (or, failing that, the bootstrap 
>> class loader)"
>>
>> To:
>>
>> "@return  the context ClassLoader for this Thread, or {@code null} 
>> indicating the caller shall use the system class loader (or, failing 
>> that, the bootstrap class loader) in place of what would otherwise be 
>> the non-null context ClassLoader."
>>
>> Because TCCL has the annoying property of decoupling behaviour (or 
>> "action at a distance") i think i can see why it was documented in 
>> such a way.
>>
>>
>> So from my perspective you are proposing a change in the specified 
>> behaviour, perhaps to something like:
>
> No I'm not because you can not specify how your caller must behave.
>
>> @return  the context ClassLoader for this Thread, or {@code null} 
>> indicating the context ClassLoader is undefined. It is recommended 
>> that ...
>>
>> Which might be fine, from a backwards compatibility perspective, i am 
>> not sure, but i have been on the receiving end of some very tricky 
>> bugs due to TCCL, hence a conservative position.
>>
>>
>> <snip>
>>>   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.
>>>
>>
>> It can if the system CL is null.
>
> The system CL can never be null.
>
> David
> -----

Then there is some dead code in ClassLoader.getSystemClassLoader() method:

     public static ClassLoader getSystemClassLoader() {
         initSystemClassLoader();
         if (scl == null) {
             return null;
         }

And also the javadoc for the method is wrong since it is implying that 
the method may return null...

Or it might be that API is designed to allow null as a return value but 
the OpenJDK implementation never returns one.

If the later is true and ServiceLoader is to be re-used in other Java 
platform implementations, then it must be coded by the API contract and 
not by the implementation details and so should check for null...

Regards, Peter

>
>> Paul.