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

[Paul Sandoz]

Hi Joe,

On Aug 28, 2012, at 7:35 AM, Joe Wang <huizhe.wang at oracle.com> wrote:
>> 
>> --
>> 
>> 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.
> 

If there is an SCE it is hard for the developer to trace. Was SL used or not? How would a developer know?

The cast from Throwable to Exception should be avoided, here is the code in SL:

        public S next() {
            if (!hasNext()) {
                throw new NoSuchElementException();
            }
            String cn = nextName;
            nextName = null;
            try {
                S p = service.cast(Class.forName(cn, true, loader)
                                   .newInstance());
                providers.put(cn, p);
                return p;
            } catch (ClassNotFoundException x) {
                fail(service,
                     "Provider " + cn + " not found");
            } catch (Throwable x) { // <--------------- this could be an instance of Error
                fail(service,
                     "Provider " + cn + " could not be instantiated: " + x,
                     x);
            }
            throw new Error();          // This cannot happen
        }

Class.forName and newInstance can throw an instance of LinkageError. I have see such errors from SL due to the incorrect class path settings.


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

It's redundant code, declare FactoryConfigurationError in the method signature if you want to declare such intent.


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

I suspect we should be conservative and try and keep as close to the existing behaviour as possible.


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

It's not quite the same order because the SchemaFactory class may have been loaded by a custom CL, so you need to do:

 ServiceLoader.load(SchemaFactory.class, classLoader);

Normally that classLoader would be the bootstrap CL (i.e. null) and then you would be correct, but things like app servers do some strange stuff, hence the code "cl = SchemaFactory.class.getClassLoader();" which seems redundant on first glance and i am guessing is there for a good reason.

Paul.