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

[Daniel Fuchs]

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