RFR: javax.xml.transform: Using ServiceLoader to load JAXP stream factories (7169894: JAXP Plugability Layer: using service loader)
Hi, Here is a new webrev in the series that addresses using ServiceLoader in JAXP for JDK 8. 7169894: JAXP Plugability Layer: using service loader This changeset addresses modification in the javax.xml.transform package. It is similar to changes proposed for the javax.xml.parsers package [1], with only a few differences due to the specificity of javax.xml.transform. <http://cr.openjdk.java.net/~dfuchs/JDK-7169894/javax.xml.transform/webrev.00/> best regards, -- daniel previous webrevs in the series: [1] javax.xml.parsers: <http://cr.openjdk.java.net/~dfuchs/JDK-7169894/javax.xml.parsers/webrev.06/> [2] javax.xml.datatype: <http://cr.openjdk.java.net/~dfuchs/JDK-7169894/javax.xml.datatype/webrev.02/> [3] javax.xml.stream <http://cr.openjdk.java.net/~dfuchs/JDK-7169894/javax.xml.stream/webrev.01/>
Hi Daniel, Looks great! I see you've added many verifications. One minor note, creationMethod is used in place of the object instance for the creationMethod.invoke call, although it's ignored for a static method anyways. Is there a reason for doing that? -Joe On 12/18/2012 8:42 AM, Daniel Fuchs wrote:
Hi,
Here is a new webrev in the series that addresses using ServiceLoader in JAXP for JDK 8.
7169894: JAXP Plugability Layer: using service loader
This changeset addresses modification in the javax.xml.transform package. It is similar to changes proposed for the javax.xml.parsers package [1], with only a few differences due to the specificity of javax.xml.transform.
<http://cr.openjdk.java.net/~dfuchs/JDK-7169894/javax.xml.transform/webrev.00/>
best regards,
-- daniel
previous webrevs in the series:
[1] javax.xml.parsers: <http://cr.openjdk.java.net/~dfuchs/JDK-7169894/javax.xml.parsers/webrev.06/>
[2] javax.xml.datatype: <http://cr.openjdk.java.net/~dfuchs/JDK-7169894/javax.xml.datatype/webrev.02/>
[3] javax.xml.stream <http://cr.openjdk.java.net/~dfuchs/JDK-7169894/javax.xml.stream/webrev.01/>
On 12/18/12 7:38 PM, Joe Wang wrote:
Hi Daniel,
Looks great! I see you've added many verifications.
One minor note, creationMethod is used in place of the object instance for the creationMethod.invoke call, although it's ignored for a static method anyways. Is there a reason for doing that? Yes sorry about that - it's my mistake. I had wrapped that call in a previous version then decided to revert to the original code but I obviously did it too fast!
Thanks for catching it - the tests didn't! -- daniel
-Joe
On 12/18/2012 8:42 AM, Daniel Fuchs wrote:
Hi,
Here is a new webrev in the series that addresses using ServiceLoader in JAXP for JDK 8.
7169894: JAXP Plugability Layer: using service loader
This changeset addresses modification in the javax.xml.transform package. It is similar to changes proposed for the javax.xml.parsers package [1], with only a few differences due to the specificity of javax.xml.transform.
<http://cr.openjdk.java.net/~dfuchs/JDK-7169894/javax.xml.transform/webrev.00/>
best regards,
-- daniel
previous webrevs in the series:
[1] javax.xml.parsers: <http://cr.openjdk.java.net/~dfuchs/JDK-7169894/javax.xml.parsers/webrev.06/>
[2] javax.xml.datatype: <http://cr.openjdk.java.net/~dfuchs/JDK-7169894/javax.xml.datatype/webrev.02/>
[3] javax.xml.stream <http://cr.openjdk.java.net/~dfuchs/JDK-7169894/javax.xml.stream/webrev.01/>
On 12/18/12 8:42 AM, Daniel Fuchs wrote:
Hi,
Here is a new webrev in the series that addresses using ServiceLoader in JAXP for JDK 8.
7169894: JAXP Plugability Layer: using service loader
This changeset addresses modification in the javax.xml.transform package. It is similar to changes proposed for the javax.xml.parsers package [1], with only a few differences due to the specificity of javax.xml.transform.
<http://cr.openjdk.java.net/~dfuchs/JDK-7169894/javax.xml.transform/webrev.00/>
In FactoryFinder.newInstanceNoServiceLoader method, L223, 226 - NoSuchMethodException will be thrown if such method doesn't exist. creationMethod will not be 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. 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. Thanks Mandy
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
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!) <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
On 12/19/2012 6:34 AM, Daniel Fuchs wrote:
<http://cr.openjdk.java.net/~dfuchs/JDK-7169894/javax.xml.transform/webrev.01/>
Thanks for the update and it looks much better. FactoryFinder L214-215: this will swallow the new TransformerFactoryConfigurationError thrown at L210. Maybe you can explicitly catch CCE that rethrow TFCE. Nit: L189-190 needs some indentation and probably better to merge them in one line. Mandy
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
participants (3)
-
Daniel Fuchs
-
Joe Wang
-
Mandy Chung