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

[Alan Bateman]

On 04/12/2012 14:42, Daniel Fuchs wrote:
> :
>
>> 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.
I see your point and happy to see that you are very careful with these 
changes. I think this just means that the wording, specifically "the 
error will be wrapped" needs to be changed so that it doesn't give the 
impression the SCE will be the cause.

> :
>
>> 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...
I think it would be neater but it's a minor comment and nothing to do 
with the main dish.

> :
>>
>> 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?
Good question, I'd suggest adding a note to JDK-7169894 for now so that 
it is as least recorded somewhere.

-Alan