RFR: javax.xml.transform: Using ServiceLoader to load JAXP stream factories (7169894: JAXP Plugability Layer: using service loader)

[Joe Wang]

On 12/19/2012 6:34 AM, Daniel Fuchs wrote:
> Hi,
>
> Please find below an updated webrev for the javax.xml.transform
> package, taking into account Mandy's & Joe's comments - namely:
>
> 1. Fixed call to creationMethod.invoke.
>
> 2. Got rid of the 4 args FactoryFinder.newInstance method.
>
> 3. Got rid of the useBSClsLoader which was always passed as 'false'.
>    (thanks Mandy!)

I agree with the change. Just to clarify, the only case where 
useBSClsLoader could be 'true' was in the old findJarServiceProvider 
method.

There might be subtle things the ServiceLoader does that could be 
different from the findJarServiceProvider process. In both processes, 
ContextClassLoader is used first, then the old process would try the 
classloader of the finder class which is the bootclassloader.

The ServiceLoader spec says: The ServiceLoader.load(service) is 
equivalent to ServiceLoader.load(service, 
Thread.currentThread().getContextClassLoader()) where "loader - The 
class loader to be used to load provider-configuration files and 
provider classes, or null if the system class loader (or, failing that, 
the bootstrap class loader) is to be used "

So we need to make sure it works fine when ContextClassLoader is null, 
or ContextClassLoader's parent is not the app class loader. I think we 
have regression tests for the scenarios. Since you've run them, I don't 
see there's any problem.

-Joe

>
> <http://cr.openjdk.java.net/~dfuchs/JDK-7169894/javax.xml.transform/webrev.01/> 
>
>
>
> -- daniel
>
> On 12/18/12 11:02 PM, Daniel Fuchs wrote:
>> On 12/18/12 9:06 PM, Mandy Chung wrote:
>>> In FactoryFinder.newInstanceNoServiceLoader method, L223, 226 -
>>> NoSuchMethodException will be thrown if such method doesn't exist.
>>> creationMethod will not be null.
>> Thanks - yes - you're right of course - no need to check for null...
>>> L236 - this change is not needed, right?  The method is a static
>>> no-arg method. You passed an additional argument creationMethod as the
>>> first parameter although it's harmless as it's ignored.
>> Oops - my bad. That's a mistake - I did too many successive changes -
>> should be creationMethod.invoke(null) of course.
>> And my tests didn't even catch it!
>>>
>>> A minor comment:
>>>   151     static<T>  T newInstance(Class<T>  type, String className,
>>> ClassLoader cl, boolean doFallback)
>>>   152         throws TransformerFactoryConfigurationError
>>>   153     {
>>>   154         return newInstance(type, className, cl, doFallback,
>>> false, false);
>>>   155     }
>>>
>>> The FactoryFinder.newInstance method 4-argument version is only 
>>> called by
>>> TransformerFactory.newInstance(String factoryClassName, ClassLoader
>>> classLoader).
>>> Perhaps you can clean this up TransformerFactory to call the
>>> Factory.newInstance
>>> method 6-argument version.
>> 3 successive boolean parameters... I hate that ;-)  Yes I think I can do
>> this cleanup...
>>
>> Thanks Mandy,
>>
>> -- daniel
>>
>>>
>>> Thanks
>>> Mandy
>>
>