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

[Joe Wang]

On 8/27/2012 6:19 AM, Alan Bateman wrote:
>  On 24/08/2012 17:52, Joe Wang 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
> I've taken a pass over this, it looks much better now (duplicate 
> FactoryFinders aside).

Duplicate indeed. The original designers were in the same team, but 
somehow they decided the wordings, class names (exception/error classes) 
could be different. I guess we have to live with it, just a bit more 
tedious work.

>
> In DocumentBuilderFactory and SAXParserFactory the javadoc reads " 
> Otherwise the default implementation is returned if it is on the 
> classpath or installed as a module". I think this statement needs to 
> be re-worked, first to remove the word "module" as it is not defined 
> here, and also because I don't think it's the classpath. If I read the 
> code correctly then initiating loader may actually be the context 
> class loader and assuming the usual delegation then the default should 
> be loaded by the boot class loader. It may be possible to borrow some 
> of the wording from the ServiceLoader javadoc to help here.

It seems we need to go over this again since it was intended for 
jdk8/jigsaw. Without modules, we don't really need to put other 
implementations over the default.  How about change it to the following:

      * Uses the service-provider loading facilities, defined by the 
{@link java.util.ServiceLoader} class, to attempt
      * to locate and load an implementation of the service. If there 
are multiple providers, the first one returned
      * by the  {@link java.util.ServiceLoader} will be instantiated and 
returned.
      *
      * If a misconfigured provider is encountered and {@link 
java.util.ServiceConfigurationError} is thrown, the error will be wrapped
      * in a {@link javax.xml.parsers.FactoryConfigurationException}.</p>


>
> In DatatypeFactory.java then it would be good to use {@link ..} or 
> {@code ..} around the error and exception types that are referenced in 
> the javadoc. Same thing in several other classes.

All updated.

>
> In src/javax/xml/datatype/FactoryFinder.java then is there any reason 
> why this class has to use Object? I realize that some of the factory 
> finders are called for several types but there are few (like this one) 
> where there is only one type involved.

No, it doesn't. FactoryFinder for datatype and transform can be 
dedicated.  It was believed that it and SecuritySupport should be copied 
for each package, I think we touched this before.  I left it alone, or 
otherwise we'd need to change all other methods in the class and also 
the transformer factory finder, for not much performance gain.

>
> DocumentBuilderFactory.java it has:
>   catch (FactoryConfigurationError e) { throw e; }
> I assume the catch is not needed, same thing in a few other places.

It's not. But since the javadoc for the method defined throws, I thought 
it's good to re-throw it within the method rather than going into the 
finder class to know that the Error will be thrown.

>
> Also in DocumentBuilderFatcory: " If ServiceConfigurationError is 
> thrown, it is interpreted as an invalid provider is encountered. The 
> process shall continue the search until all are exhausted."
> I think this could be re-worded, minimally "as an invalid" to "as if 
> an invalid".  Also "until all are exhausted" isn't completely clear to 
> me, maybe it needs to re-iterate that it falls back to the system default.

Somehow I managed missing these two factories when I made the changes as 
we discussed. I actually went through all of the changes one more time, 
but still missed them.  Corrected now.

>
> Minor nit in src/javax/xml/validation/FactoryConfigurationError.java 
> but it seems to be using 2-space indents rather than 4-space indents.

Fixed. I guess I need to re-format every time I make a copy of an 
existing class :)

>
> I think one thing that would be really useful here is to get a summary 
> of the behavioral and API changes that this changes brings. 
> SchemeFactory now throws FactoryConfigurationError whereas previously 
> errors were ignored. It would be good to summarize the others.

Yes, I think we'll do that in the MR. There isn't a general place in the 
Javadoc for us to do that.

--Joe

>
> -Alan.
>
>
>