RFR [JDK8]: 7169894: JAXP Plugability Layer: using service loader

[Paul Sandoz]

Hi Joe,

This is starting to look cleaner.

--

datatype/FactoryFinder.java:

 244         } catch (ServiceConfigurationError e) {
 245             throw new DatatypeConfigurationException(e.getMessage(), e.getCause());

You are munging the message of the exception and it's cause. Perhaps it would be better just to pass along the SCE as the cause, that way it is better identified that SL is being used when an error occurs.

--

There is inconsistency of the JavaDoc of DocumentBuilderFactory (some others) and DataTypeFactory. The former also makes reference to modules. I guess you just need to sync up the JavaDoc.

--

parsers/DocumentBuilderFactory.java

121     public static DocumentBuilderFactory newInstance() {
 122         try {
 123             return (DocumentBuilderFactory) FactoryFinder.find(DocumentBuilderFactory.class,
 124                     /* The default property name according to the JAXP spec */
 125                     "javax.xml.parsers.DocumentBuilderFactory",
 126                     /* The fallback implementation class name */
 127                     "com.sun.org.apache.xerces.internal.jaxp.DocumentBuilderFactoryImpl");
 128         } catch (FactoryConfigurationError e) {
 129             throw e;

 130         }

You are just re-throwing.

--

parsers/FactoryFinder.java

 280         } catch (ServiceConfigurationError e) {
 281             throw new FactoryConfigurationError((Exception) e.getCause(), e.getMessage());

A CCE could occur. Like above i think you are better off passing the SCE as the cause.

Same goes for some other factory-based classes.

--

parsers/SAXParserFactory.java

 125     public static SAXParserFactory newInstance() 
 126     {
 127         try{
 128             return (SAXParserFactory) FactoryFinder.find(SAXParserFactory.class,
 129                     /* The default property name according to the JAXP spec */
 130                     "javax.xml.parsers.SAXParserFactory",
 131                     /* The fallback implementation class name */
 132                     "com.sun.org.apache.xerces.internal.jaxp.SAXParserFactoryImpl");
 133         } catch (FactoryConfigurationError e) {
 134             throw e;

 135         }
 136     }

You are just re-throwing.

--

validation/SchemaFactoryFinder.java

 212         SchemaFactory f = null;
 213         try {
 214             f = new SchemaFactoryFinder(cl).newFactory(schemaLanguage);
 215         } catch (FactoryConfigurationError e) {
 216             throw e;
 217         }
You are just re-throwing.

--

SchemaFactoryFinder and XPathFactoryFinder

It seems the error behaviour has changed. AFAICT previously any exceptions would be swallowed and null would be returned. If there is an SCE then an exception will now be propagated. This may be OK though, just not totally sure.

When using SL you are ignoring the classloader passed into the constructor. I think you may have to pass the classloader as an argument to SL because of the selection:

 201     public static final SchemaFactory newInstance(String schemaLanguage) 
 202     {
 203         ClassLoader cl;
 204         cl = ss.getContextClassLoader();
 205 
 206         if (cl == null) {
 207             //cl = ClassLoader.getSystemClassLoader();
 208             //use the current class loader
 209             cl = SchemaFactory.class.getClassLoader();
 210         }

Given some of the weird class loader things app servers do i am guessing SchemaFactory may not always be loaded by the system class loader.

Paul.


On Aug 24, 2012, at 6:52 PM, Joe Wang <huizhe.wang at oracle.com> wrote:

> Hi,
> 
> Here is a modified patch: http://cr.openjdk.java.net/~joehw/jdk8/7169894/webrev/
> 
> The factory finders contain some format changes, a NetBeans format work. The real changes are as the follows:
> 
> 1) In factory classes: reinstated the implementation resolution mechanism, the 3rd step mainly
> 
> 2) In factory finders: replaced findJarServiceProvider with findServiceProvider
> 
> 3) In factory finders: removed ConfigurationError class, using FactoryConfigurationError instead
> 
> Please review.
> 
> Thanks,
> Joe