CFR: javax.xml.parsers: Using ServiceLoader to load JAXP parser factories (7169894: JAXP Plugability Layer: using service loader)
daniel.fuchs at oracle.com
Thu Dec 6 11:22:23 UTC 2012
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
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!
> On 12/5/2012 8:17 AM, Daniel Fuchs wrote:
>> 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.
>> * 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:
>>>> 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 
>>>> 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.
>>> 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
>>> 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.
More information about the core-libs-dev