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

[Joe Wang]

On 8/27/2012 6:19 AM, Paul Sandoz wrote:
> Hi Joe,
>
> This is starting to look cleaner.

Yeah, if Alan hasn't asked, I'd sooner keep them as they were :)  JAXP 
is old, I never fancied getting those formats corrected.  But I can't do 
this much to classes I'd update, but probably not to the impl classes in 
sync with Apache, even if it might mean poor format.

>
> --
>
> 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.

None of the ConfigureError classes in other packages accept Error or 
Throwable as did DataType (and this one is an Exception!)
So instead of making changes to the ConfigureError classes, I wrapped 
the ServiceConfigurationError in jaxp configuration errors, and in this 
case (Datatype), a datatype configuration exception

It should be very rare to get a ServiceConfigurationError that would 
indicate a serious error in a jar configuration, basically, a non-usable 
implementation.  So I think we don't have to stick with the 
ServiceConfigurationError.

>
> --
>
> 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.

Updated.

>
> --
>
> 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.

I did. It's not necessary technically. But since the javadoc for the 
method defined it, I thought it's good to re-throw it within the method, 
or repeat it, it's also making sure FactoryFinder indeed throw the error.

> --
>
> 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.

Yes, it's changed.  We discussed this issue before, all but Datatype 
swallowed any error out of the 3rd step and they all fall back to the 
default impl in the 4th step.  In a recent conversation with Alan, he 
wanted to see the error caught, and I believe you did too.  I didn't 
think we needed to, but I'm okay both ways since it shall very rarely 
happen. Even if it does, it should have been fixed by an impl developer 
long before it's out.

This is also one of those 'differences' in the api impl that kept 
troubling us :)


>
> 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.

I did. This is the same order as in the ServiceLoader.  So I thought it 
didn't really make any difference.  The only case where a user supplied 
classloader may be used is when a factory class name is also explicitly 
specified.

-Joe

>
> 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