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

[Daniel Fuchs]

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