RFR: javax.xml.stream: Using ServiceLoader to load JAXP stream factories (7169894: JAXP Plugability Layer: using service loader)

[Joe Wang]

On 12/18/2012 2:23 PM, Daniel Fuchs wrote:
> On 12/18/12 8:00 PM, Joe Wang wrote:
>>
>>
>> On 12/18/2012 10:00 AM, Daniel Fuchs wrote:
>>> On 12/18/12 6:29 PM, Joe Wang wrote:
>>>> Hi Daniel,
>>>>
>>>> The call: T provider = findServiceProvider(type) in static <T> T
>>>> find(Class<T> type, String factoryId, ClassLoader cl, String
>>>> fallbackClassName) ignored factoryId, and assumed it's the same as
>>>> type.getName(). I looked back I had the same bug in my original patch.
>>>
>>> I don't think that's a bug in the new code - but rather possibly a
>>> bug in the old code ;-).
>>
>> The old code did not have the problem since it's reading 
>> "META-INF/services/" + factoryId directly.
>>
>>> There's no way you can pass a property name to the ServiceLoader.
>>
>> True. That's why it looked a bit awkward since 'factoryId' is not 
>> passed in as did before, as in findJarServiceProvider(factoryId).
>>
>>>
>>> The JAR Specification says:
>>>
>>> "A service provider identifies itself by placing a 
>>> provider-configuration file in the resource directory 
>>> META-INF/services. The file's name should consist of the 
>>> fully-qualified name of the abstract service class."
>>>
>>> So I think the current code is correct in ignoring factoryId - because
>>> according to the spec the file name should be the same as the
>>> abstract class name.
>>
>> So the StAX API implied that factoryId could be anything, but that 
>> would violate the JAR specification. In other words, it would only 
>> work as intended if the factoryId is specified as a System Property, 
>> or in stax.properties or jaxp.properties.
>>
>> I would think we should return an error if factoryId != 
>> type.getName() before the call "findServiceProvider(type)".
>
> In fact I toyed with the idea of skipping the call to 
> findServiceProvider(type) when factoryId != type.getName().
> I am not sure whether we should return an error or the skip directly 
> to returning the default implementation.
>
> I mean - if a user tried to load the factory specified by foo.bar, and 
> foo.bar is not defined, shouldn't we return
> the default implementation? I think that's what was happening before...

It's different. If 'foo.bar' is specified but not found, it indicates a 
configuration error. If the factory falls back to an impl by the default 
factory id, it would serve to hide the error. Note that 
newInstance/newFactory with a factoryId parameter do not fall back to 
the default implementation.

-Joe

>
> But if we return the default implementation - shouldn't any Service 
> Provider loadable through ServiceLoader take
> precedence?
>
> When the parameter is a classname - then the meaning is clear. Either 
> you can load the class or you can't.
> When it's a factoryId - and the doc says "it is the  same as a System 
> property" - then shouldn't the code
> behave as if it were a System property?
>
> -- daniel
>
>>
>> -Joe
>>
>>>
>>>
>>> -- daniel
>>>
>>>>
>>>> -Joe
>>>>
>>>> On 12/18/2012 8:39 AM, Daniel Fuchs wrote:
>>>>> Hi,
>>>>>
>>>>> Thanks for the review.
>>>>> I updated the webrev to keep track of your suggested change.
>>>>> <http://cr.openjdk.java.net/~dfuchs/JDK-7169894/javax.xml.stream/webrev.01/> 
>>>>>
>>>>>
>>>>>
>>>>> -- daniel
>>>>>
>>>>> On 12/18/12 4:00 PM, Alan Bateman wrote:
>>>>>>
>>>>>> I looked through this installment and aside from an aside from an
>>>>>> alignment issue at lines 101-102 in XMLEventFactory.java then it 
>>>>>> looks
>>>>>> good to me.
>>>>>>
>>>>>> Also thank you again for being so careful as you work through 
>>>>>> each of
>>>>>> these areas.
>>>>>>
>>>>>> -Alan
>>>>>
>>>
>