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

[Daniel Fuchs]

Hi Alan,

Thanks a lot for your comments - some clarifications inline.

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.

OK - will do.

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

I have refrained to do that because I think there might be code that
brutally does things like (Exception) error.getCause();
(I've seen this kind of pattern in the JAXP code - although
I don't remember exactly where...)
The reason is that FactoryConfigurationError is for some reason
obviously supposed to wrap only exceptions - as its constructor will
take only exceptions - and not throwables, and then overrides
getCause() to return a locally cached variable of type Exception - see
<http://hg.openjdk.java.net/jdk8/tl/jaxp/raw-file/tip/src/javax/xml/parsers/FactoryConfigurationError.java>

Using initCause() would therefore have no effect, unless the
implementation of javax.xml.parsers.FactoryConfigurationError is
changed to accept Throwable as cause - which could - I think have
undesirable side effects on existing code.

My guess is that this is an oddity inherited from the time when
Cause was not defined at Throwable level - but managed locally
on specific exceptions classes instead.
I am fearing that changing the implementation of getCause()
to allow setting the SCE  directly as the cause might break 
compatibility, causing some ClassCastException to be thrown here
and there.


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

Will do.

> Minor nit is that the updates to DocumentBuilderFactory's class
> description requires a really wide screen compared to the rest of the
> javadoc.

Will fix.

> Minor implementation suggestion for findServiceProvider is that you can
> use for-each to iterate through the providers.

I don't mind. Do you think it's better? I'll make the changes and send
an updated webrev...

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

OK - but in what form? In the webrev header? Or do you think I should
add // comments in the code to make explicit the behavioral changes?
Or do you mean I should collect & gather them in some separate text
doc that will help writing the RN?

>
> -Alan.