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

Daniel Fuchs daniel.fuchs at oracle.com
Thu Dec 6 11:22:23 UTC 2012

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 DocumentBuilderFactory 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 DocumentBuilderFactory 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 DocumentBuilderFactory 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.

More information about the core-libs-dev mailing list