RFR: javax.xml.stream: Using ServiceLoader to load JAXP stream factories (7169894: JAXP Plugability Layer: using service loader)
Daniel Fuchs
daniel.fuchs at oracle.com
Tue Jan 8 12:20:47 UTC 2013
On 1/8/13 5:13 AM, Mandy Chung wrote:
> Hi Daniel,
>
> I also agree with option 3 be the best option among them. Joe's
> suggestion to throw an exception if factoryId is not the base service
> type is good so that any existing application depending on that behavior
> will catch this change. Are you going to revise the spec - there is
> statement saying that the newFactory method is the same behavior as
> newInstance method which is weakly specified.
>
> 139 * No changes in behavior are defined by this replacement method
> relative
> 140 * to the deprecated method.
>
>
> It'd be good to add some tests to cover these APIs if there is none.
> Otherwise, the change looks good.
> Mandy
Right - thanks Mandy.
I've clarified the spec. I also documented an additional step, which
was done by the implementation but not documented - which is that
the stream factory will also look up for properties in jaxp.properties
if stax.properties was not found.
webrev:
<http://cr.openjdk.java.net/~dfuchs/JDK-7169894/javax.xml.stream/webrev.03/>
I have generated a spec diff for easier reading of the doc changes:
<http://cr.openjdk.java.net/~dfuchs/JDK-7169894/javax.xml.stream/specdiff.03/overview-summary.html>
regards,
-- daniel
>
> On 1/7/2013 2:34 AM, Daniel Fuchs wrote:
>> Thanks Joe.
>>
>> To make things clear I have pushed a revised webrev with solution 3.
>> <http://cr.openjdk.java.net/~dfuchs/JDK-7169894/javax.xml.stream/webrev.02.3/>
>>
>>
>> The lines of interest are in FactoryFinder.java - right hand side:
>>
>> 267 if (type.getName().equals(factoryId)) {
>> 268 // Try Jar Service Provider Mechanism
>> 269 final T provider = findServiceProvider(type);
>> 270 if (provider != null) {
>> 271 return provider;
>> 272 }
>> 273 } else {
>> 274 // We're in the case where a 'custom' factoryId was
>> provided,
>> 275 // and in every case where that happens, we expect that
>> 276 // fallbackClassName will be null.
>> 277 assert fallbackClassName == null;
>> 278 }
>>
>> regards,
>>
>> -- daniel
>>
>>
>> On 1/4/13 7:51 PM, Joe Wang wrote:
>>> Hi Daniel,
>>>
>>> Yes, I agree with 3. As I said before, we should return an error if
>>> factoryId != type.getName() since it indicates a configuration error.
>>> Scenario 4 does exist, but it's beyond the current spec. Such an impl
>>> should not use the default API.
>>>
>>> The StAX spec is not always clear. My interpretation of the definition
>>> of factoryId to be the "same as a property name" is that the author
>>> basically was saying that it's equivalent to a name as in a property,
>>> not the "value", therefore different from the DOM/SAX API -- a bad
>>> choice I would think.
>>>
>>> Thanks,
>>> Joe
>>>
>>> On 1/4/2013 6:31 AM, Daniel Fuchs wrote:
>>>> Hi guys,
>>>>
>>>> Happy new year to you all! And apologies for this long email :-)
>>>>
>>>> I think we still haven't converged on this issue in
>>>> javax.xml.stream - so let me make a recap.
>>>>
>>>> The issue is for the newInstance/newFactory methods
>>>> that take a factoryId parameter, in the factories for
>>>> the javax.xml.stream package:
>>>>
>>>> [
>>>> <http://cr.openjdk.java.net/~dfuchs/JDK-7169894/javax.xml.stream/webrev.01/>
>>>>
>>>>
>>>> FactoryFinder.java: line 267-268 right hand side. ]
>>>>
>>>>
>>>> recap of the issue:
>>>> -------------------
>>>>
>>>> In case the provided factoryId did not correspond to any
>>>> System/JAXP/StAX property, the old code used to look
>>>> for a service in META-INF/services using the factoryId as the
>>>> name of the file for looking up the implementation class.
>>>> In case factoryId was not the same as the base service
>>>> class (or a subclass of it) it still would have worked
>>>> although it went in contradiction to the Jar Specification
>>>> mentioned in the javadoc, since the Jar Specification clearly
>>>> states that the file name should be the fully qualified name
>>>> of the base service class.
>>>>
>>>> Since we're going to replace the code that looked up for
>>>> a service in META-INF with a call to ServiceLoader, we can
>>>> no longer fully preserve that old behavior, because ServiceLoader
>>>> only takes a base service class (and no arbitrary file name).
>>>>
>>>> what choices do we have?
>>>> ------------------------
>>>>
>>>> The question is therefore how we want to change this.
>>>> I think we have 4 (exclusive) possibilities.
>>>>
>>>> In the case where a factoryId is provided, but no
>>>> System/JAXP/StAX property by that name has been found,
>>>> we could choose to either:
>>>>
>>>> 1. Always call ServiceLoader.load(type) where type is the
>>>> service base class.
>>>>
>>>> 2. Never call ServiceLoader.load(type)
>>>>
>>>> 3. Only call ServiceLoader.load(type) when the provided
>>>> factoryId is equal to type.getName()
>>>>
>>>> 4. If factoryId is equal to type.getName(), call
>>>> ServiceLoader.load(type), otherwise,
>>>> attempt to load factoryId as a class - and if that
>>>> class is a subclass of 'type' then call
>>>> ServiceLoader.load for that subclass.
>>>>
>>>> pros & cons:
>>>> ------------
>>>>
>>>> Here is a list of pros & cons for each of these options:
>>>>
>>>> 1. is the naive implementation I originally proposed.
>>>> Pros:
>>>> - P1.1: simple
>>>> - P1.2: no change in behavior if factoryId == type.getName()
>>>> Cons:
>>>> - C1.1: may find no provider when the old code used to
>>>> find one, (case where factoryId="foo", there's a
>>>> provider for "foo" and there's no provider for type.getName())
>>>> - C1.2: may find a provider when the old code used to
>>>> find none (case where factoryId="foo", there's no provider
>>>> for foo, but there's a provider for type.getName())
>>>> - C1.3: may find a different provider than what the old
>>>> code use to find (case where factoryId="foo", there's a
>>>> provider for "foo" and there's also a provider for
>>>> type.getName())
>>>>
>>>> 2. is more drastic: if you specify a property - then the property
>>>> has to be defined somewhere for newInstance/newFactory to
>>>> succeed.
>>>> Pros:
>>>> - P2.1: simple
>>>> Cons:
>>>> - C2.1: there's a change of behavior even when
>>>> factoryId == type.getName() and no property by that
>>>> name is defined.
>>>> - C2.2: in all cases where the old code used to find a
>>>> provider by looking at META-INF/services, the new code
>>>> will find nothing.
>>>> Although it's the most simple - it's probably the most risky
>>>> in terms of compatibility.
>>>>
>>>> 3. Same as 1. except that C1.2 and C1.3 are removed.
>>>>
>>>> 4. Same as 3. except that it's more complex (so P1.1 is
>>>> removed) and that C1.1 will not occur in the case
>>>> where factoryId is the name of a subclass of 'type'
>>>>
>>>> In conclusion:
>>>> --------------
>>>>
>>>> Since the original spec only says that factoryId is
>>>> "same as property" - it's very difficult to figure out
>>>> which of these 4 options is the closer to the behavior
>>>> intended by the spec.
>>>>
>>>> I personally think that the case where factoryId="foo",
>>>> and no property "foo" is defined, but a provider exists for
>>>> "foo" and an implementation is returned, is a bug - since
>>>> it's in contradiction with the Jar Specification mentioned
>>>> in the spec which clearly states that "foo" should
>>>> have been a service class name.
>>>>
>>>> So although 4. is the implementation that would offer the
>>>> greater compatibility with existing code, I am sorely
>>>> tempted to recommend that we do 3. and clarify
>>>> the spec on this point.
>>>> (3: Only call ServiceLoader.load(type) when the provided
>>>> factoryId is equal to type.getName())
>>>>
>>>> Best regards,
>>>>
>>>> -- daniel
>>>>
>>>> On 12/19/12 10:45 AM, Daniel Fuchs wrote:
>>>>> On 12/19/12 12:10 AM, Joe Wang wrote:
>>>>>> 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.
>>>>> Yes - I fully agree with that.
>>>>>> Note that newInstance/newFactory with a factoryId parameter do not
>>>>>> fall back to the default implementation.
>>>>> Ahh! Yes I missed that. When called from newInstance with a factoryId
>>>>> parameter the fallbackClassname parameter
>>>>> is null...
>>>>>
>>>>> So should we still call ServiceLoader when fallbackClassname is
>>>>> null and
>>>>> factoryId is type.getName()? It would be more
>>>>> backward compatible - since previously it was looking in the jars and
>>>>> found (valid) providers registered with that name.
>>>>>
>>>>> On the other hand we could alter the spec to say that if no property
>>>>> with the factoryId name is found - then
>>>>> no fallback loading is perform (either on ServiceLoader or
>>>>> system-default implementation) and an error is thrown.
>>>>>
>>>>> -- daniel
>>>>
>>
More information about the core-libs-dev
mailing list