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

[Joe Wang]

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
>