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

Joe Wang huizhe.wang at oracle.com
Fri Jan 4 18:51:33 UTC 2013

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