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

[Joe Wang]

Alan, Paul,

While I was writing a summery as you suggested below, I noticed an issue 
with using ServiceLoader.  I was trying to use the word 'delegate' but 
found the ServiceLoader might be doing sth. different than the original 
jaxp process.

Here's the spec:
The ServiceLoader.load(service) is equivalent to 
ServiceLoader.load(service, 
Thread.currentThread().getContextClassLoader()) where "loader - The 
class loader to be used to load provider-configuration files and 
provider classes, or null if the system class loader (or, failing that, 
the bootstrap class loader) is to be used "


Somehow, our earlier discussion concluded that the original process I 
created, where the context class loader is tried first, if no provider 
found, bootstrap classloader will be tried next, could be 'delegated' to 
the ServiceLoader.  But the above didn't really showed that. When I 
looked at the code, it actually simply returns false if 
loader.getResources fails to find a provider.  Also, it calls 
ClassLoader.getSystemResources when loader=null.  The later would be a 
regression to CR6723276 if we had called ServiceLoader.load(service, 
loader) with loader=null.

Am I missing sth. about ServiceLoader?

--Joe

On 8/29/2012 3:27 AM, Alan Bateman wrote:
> On 28/08/2012 05:57, Joe Wang wrote:
>> :
>>
>>>
>>> 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>
> I think it should allow for the possibility that the default factory 
> is itself installed as a service provider. To that end, I think the 
> original proposed working was in the right direction, it's just that 
> it used terms "classpath" and "modules" too loosely and didn't define 
> what the initiating loader was. Here is some suggested wording to chew on:
>
> "Installed providers are loaded using the service-provider loading 
> facility defined by the {@link ServiceLoader} class. Providers are 
> loaded using the current thread's context class loader. If the context 
> class loader is {@code null} then the system class loader if used. The 
> first service provider to be instantiated that is not the default 
> provider is returned. If the only service provider to be located is 
> the default provider then it is instantiated and returned.
>
> If a {@link ServiceConfigurationError} is encountered when locating or 
> iterating over the providers then this causes {@link 
> FactoryConfigurationException} to be thrown, with the {@code 
> ServiceConfigurationError} as its cause."
>
>>
>> :
>>
>>>
>>> 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.
> Okay, but I do think this needs clean-up at some point.
>
>
>>
>>>
>>> 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.
> I see Paul's has picked up on this in his comments too.
>
>
>>
>> :
>>
>>>
>>> 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.
> I understand, I'm just saying think the list would be useful for the 
> review here too.  I suggest this because there are subtle differences 
> between the datatype finder and the other finders, also the 
> SchemaFactory is being changed to throw an error that it previously 
> didn't define, etc. You'll need this for for JCP MR too but for now I 
> think it is important for the reviewers here to have the summary so 
> that we know what we are reviewing.
>
> -Alan
>
>
>