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

[Daniel Fuchs]

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

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