Reviewer and committer request for 7198496

[David Holmes]

Not wanting to beat a dead-horse but ...

On 5/10/2012 6:56 AM, Alan Bateman wrote:
> On 04/10/2012 15:39, Paul Sandoz wrote:
>> :
>> I thought that was the case too, my first webrev had the if/else
>> removed but then i updated it after i read the JavaDoc of
>> ClassLoader.getSystemClassLoader():
>>
>> Returns: The system ClassLoader for delegation, or null if none
>>
>> Paul.
> Yes, the spec allows there to be no system class loader.

The @return claims it can be null, but if you read the rest of the spec 
that appears to be impossible - either the system loader has been 
created, in which case it is returned, else it is created. The creation 
is either successful or throws an exception. I don't see anywhere a null 
can possibly be returned - either in the spec or implementation.

Cheers,
David
---------

> So my take on ServiceLoader is that the changes in your updated webrev
> are okay. Code that wants to allow for service providers to be be
> installed on the class path or located via the TCCL should use the load
> methods. Code that wants to restrict things to only service providers
> that in the extensions directory or the boot class path needs to use
> loadInstalled instead. It's hard to know how common the
> TCCL=null+load(S) or load(S,null) cases are but the spec seems clear and
> with the changes then I don't think there should be any issue.
>
> As regards the Thread set/getTCCL methods then I think the null case is
> a bit of a corner case. The methods specify the intended usage but can't
> of course enforce it. My guess is that for regular applications is not
> an issue,the primordial thread starts out with it set to to the system
> class loader and other threads inherit it. I also suspect this is a
> non-issue for the EE apps too as I gather it's always set there because
> there are multiple contexts running in the same container. There can be
> security or GC reasons to reset it to null but that should be rare, at
> least outside of the JDK.
>
> -Alan
>
>