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

[Joe Wang]

On 6/21/2012 2:35 AM, Paul Sandoz wrote:
> Hi Joe,
>
> It looks a good start.

Thanks for the detailed review!

>
> There is duplication in the 6 factory finding classes, can some of it 
> be consolidated in one shared factory helper class?

The duplication of FactoryFinder and SecuritySupport classes was 
determined by the security team at the time in that these classes should 
not be made public.  The same existed in the RI. In a recent security 
change,  we removed 52 duplicated classes and used package restriction 
instead. I could consult security if we want to do that.

>
> Taking javax.xml.datatyoe.FactoryFinder as an example:
>
> When iterating over the service instances you are catching 
> ConfigurationError
>
>   264         ServiceLoader loader = ServiceLoader.load(serviceClass, cl);
>   265         final Iterator providers = loader.iterator();
>   266         while (providers.hasNext()) {
>   267             try {
>   268                 Object provider = providers.next();
>   269                 if (provider.getClass().getName().contains(fallbackClassName)) {
>   270                     defaultProvider = provider;
>   271                 } else {
>   272                     return provider;
>   273                 }
>   274             } catch (ConfigurationError e) {
>   275                 // This can happen because of class loader mismatch or any other reason.
>   276                 // log and continue to next one
>   277                 if (debug) {
>   278                     dPrint("The provider can not be instantiated due to: " + e + ".");
>   279                 }
>   280             }
>   281         }
> Did you mean ServiceConfigurationError?

ConfigurationError is internally defined. It's a contract between the 
Factory and FactoryFinder classes.  It seems to me it was a result of 
sharing the FactoryFinders and different exception types defined in the 
spec for different factories.

>
> IIUC the previous code parsed a META-INF/services file and picked the 
> first entry if present and attempt to instantiate that.
>
> I gather the approach you want to achieve with ServiceLoader is to 
> "keep on trucking". In addition if nothing but an instance of the 
> default service provider class is obtained then use that. From what i 
> can tell the "fallbackClassName" parameter is a fully qualified class 
> name so you need to do getName().equals(fallbackClassName).
>
> An alternative approach is to always assume that the default service 
> provider class is never registered in META-INF/services or in module 
> declaration. It should simplify things.

JAXP is an supported endorsed technology.  It can be placed in the 
endorsed directory or bootclasspath to replace the default impl.  When a 
3rd party impl is on the class path however, it should take preference 
over the default impl, thus the check for the one loaded by the context 
class loader.   The ServiceLoader however,  is doing a lot more than the 
original META-INF/services process. It's finding providers installed "in 
the form of extensions" and placed on the class path, it therefore 
always loads JAXP whether it's in the endorsed directory or 
bootclasspath. That's the reason I cached it as default impl intended to 
be used to replace that in the JDK.  When a 3rd party impl is on the 
classpath, we still need to let it take preference.

>
> You are first trying to use ServiceLoader with the thread context 
> class loader, then if that fails using the system class loader (since 
> FactoryFinder.class.getCLassLoader() == null).
>
>   234     private static Object findServiceProvider(String factoryId, String fallbackClassName)
>   235         throws ConfigurationError
>   236     {
>   237         final Class<?>  serviceClass;
>   238         try {
>   239             serviceClass = Class.forName(factoryId);
>   240         } catch (ClassNotFoundException e) {
>   241             throw new ConfigurationError("Unable to load " + factoryId, e);
>   242         }
>   243
>   244         Object provider = null;
>   245
>   246         // First try the Context ClassLoader
>   247         ClassLoader cl = ss.getContextClassLoader();
>   248         if (cl != null) {
>   249             provider = findProvider(serviceClass, cl, fallbackClassName);
>   250             if (provider != null)  return provider;
>   251         }
>   252
>   253         // If no provider found then try the current ClassLoader
>   254         provider = findProvider(serviceClass, FactoryFinder.class.getClassLoader(), fallbackClassName);
>   255         if (provider != null)  return provider;
>   256
>   257         return null;
>   258     }
> I am wondering if we can just get away with using 
> ServiceLoader.load(serviceClass), possibly not given the current 
> implemented (but not documented) behavior. In any case we should 
> document the class loaders being used, and in what order, with 
> ServiceLoader.

Yes, that's when I observed too that the ServiceLoader is doing more as 
I described above.

>
> I am not so sure about the "keep on trucking" approach when iterating 
> over service instances. The service mechanism is being used to 
> register a factory class that is a service provider class. If the 
> first item in the service instance iterator cannot be instantiated it 
> signals a configuration error.

As I mentioned above, the first found can be jaxp RI.

>
> --
>
> I think the use of ServiceLoader by JAXP is really good input to 
> improving ServiceLoader e.g.:
>
>   
> ServiceLoader.withDefault(defaultServiceProviderClass).load(serviceInterface);
>
> or
>
>   MyService serviceInstance = 
> ServiceLoader.withDefault(defaultServiceProviderClass).
>           first(serviceInterfaceClass);
>
> You could emulate this with your own shared factory helper class.

That would be nice.  One other thing I noticed was that .load returns an 
instance. There's a situation where a static method or constructor with 
a parameter instead of the default constructor may be called to initiate 
the provider.

-Joe

>
> Paul.
>
>
>
> On Jun 21, 2012, at 9:55 AM, Joe Wang wrote:
>
>> Hi,
>>
>> This is a patch to replace the manual process in the 3rd step of the 
>> JAXP implementation resolution mechanism with ServiceLoader.  Please 
>> see http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7169894 for 
>> more details about the issue.
>>
>> Note that FactoryFinder is duplicated for each JAXP subpackage. The 
>> ones in the following packages are the same:
>> /jaxp-api/src/javax/xml/datatype/FactoryFinder.java
>> /jaxp-api/src/javax/xml/parsers/FactoryFinder.java
>> /jaxp-api/src/javax/xml/stream/FactoryFinder.java
>> /jaxp-api/src/javax/xml/transform/FactoryFinder.java
>>
>> The following two are similar except that they perform Schema 
>> Language or Object Model support check:
>> /jaxp-api/src/javax/xml/validation/SchemaFactoryFinder.java
>> /jaxp-api/src/javax/xml/xpath/XPathFactoryFinder.java
>>
>> It's a bit rush since I have only had time to test regular JDK using 
>> JDK 1.6.0_27.  Further test on jigsaw is needed.
>> All jaxp unit/SQE tests are passed. But TCK test is still running. 
>>  So please take this as an initial review.
>>
>> webrev:
>> http://cr.openjdk.java.net/~joehw/jdk8/7169894/webrev/ 
>> <http://cr.openjdk.java.net/%7Ejoehw/jdk8/7169894/webrev/>
>>
>> Thanks,
>> Joe
>>
>>
>