CFR: javax.xml.parsers: Using ServiceLoader to load JAXP parser factories (7169894: JAXP Plugability Layer: using service loader)

[Daniel Fuchs]

Hi Mandy,

On 12/5/12 10:55 PM, Mandy Chung wrote:
> Hi Daniel,
>
> Looks good to me.  One question - the last bullet in the search order
> covers the default implementation case:
>    "Platform default <code>DocumentBuilderFactory</code> instance."
>
> Would it make sense to merge the statement "Otherwise, the default
> implementation, if present, is returned" with the above statement?  e.g.
> "Default implementation of <code>DocumentBuilderFactory</code>  if
> present".

Good point - the description matches the internal implementation, but
maybe too literally:

ServiceLoader.load() may return the default implementation - or
may return null - hence the text 'the default implementation, if 
present, is returned.'

If ServiceLoader.load() returns null, then the algorithm will again
try to instantiate the default implementation - usually using
Class.forName with the TCCL - hence the last bullet:
'Platform default <code>DocumentBuilderFactory</code> instance.'

This last step is necessary because the default implementation
may be present without being accessible through a service
provider (that's the default configuration: in JDK 8 - with no
user configuration, ServiceLoader.load() will never instantiate
the default implementation)


However - from a user point of view - I don't think it makes sense
to differentiate these two cases: As a user of the JAXP parser factories
I won't really care how the default implementation is instantiated,
will I? So indeed - I think we should merge the two!

Thanks,

-- daniel


>
> Mandy
>
> On 12/5/2012 8:17 AM, Daniel Fuchs wrote:
>> Hi,
>>
>> Please find below a revised version of the changes for
>> the javax.xml.parsers package.
>>
>> It hopefully incorporates all comments I have received so far.
>>
>> <http://cr.openjdk.java.net/~dfuchs/JDK-7169894/javax.xml.parsers/webrev.01/>
>>
>>
>> * better wording/formating for Javadoc changes
>> * using for( : ) syntax in findServiceProvider
>> * improved // comments explaining why the additional layer of
>>   RuntimeException is needed when wrapping ServiceConfigurationError.
>>
>> best regards,
>>
>> -- daniel
>>
>> On 12/4/12 2:54 PM, Alan Bateman wrote:
>>> On 03/12/2012 19:04, Daniel Fuchs wrote:
>>>> Hi,
>>>>
>>>> This is a first webrev in a series that addresses a change intended
>>>> for JDK 8:
>>>>
>>>> 7169894: JAXP Plugability Layer: using service loader
>>>>
>>>> I've been building up on Joe's work and Paul & Alan comments
>>>> from an earlier discussion thread [1]
>>>>
>>>> This here addresses changes in the javax.xml.parsers
>>>> package for the SAXParserFactory and DocumentBuilderFactory - and
>>>> more specifically their no-argument newInstance() method.
>>>>
>>>> This change replaces the custom code that was reading the
>>>> META-INF/services/ resources by a simple call to ServiceLoader.
>>>>
>>>> <http://cr.openjdk.java.net/~dfuchs/JDK-7169894/javax.xml.parsers/webrev.00/>
>>>>
>>>>
>>> Thank you very much for taking this one on. I think your approach to
>>> take javax.xml.parsers on its own is good as the previous review rounds
>>> got really stuck in the mire due to the number of factories, code
>>> duplication, and subtle specification and implementation differences.
>>>
>>> In the class descriptions it suggests that the default implementation
>>> may be "installed as a module" but we aren't there yet so I'm not sure
>>> about using the term "module". I think it should be okay to say
>>> "installed as an extension" as ServiceLoader uses this term.
>>>
>>> The class description also suggests that a SCE will be wrapped as a
>>> FactoryConfigurationException but in FactoryFinder I see that it actual
>>> wraps a RuntimeException. I think you can just use the no-arg
>>> constructor then then use initCause to set the cause to the SCE.
>>>
>>> Also the statement "If a misconfigured provider is encountered and SCE
>>> is thrown" can probably be changed to "If SCE is thrown" as it can be
>>> thrown for other reasons too.
>>>
>>> Minor nit is that the updates to DocumentBuilderFactory's class
>>> description requires a really wide screen compared to the rest of the
>>> javadoc.
>>>
>>> Minor implementation suggestion for findServiceProvider is that you can
>>> use for-each to iterate through the providers.
>>>
>>> Otherwise I think you've captured all the points of feedback from the
>>> original review.
>>>
>>> Finally one suggestion is to make keep notes on the "before & after"
>>> behavior, this is something that will be important for release and
>>> compatibility notes.
>>>
>>> -Alan.
>>>
>>>
>>>
>>>
>>>
>>