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

Daniel Fuchs daniel.fuchs at oracle.com
Tue Dec 18 22:23:49 UTC 2012

On 12/18/12 8:00 PM, Joe Wang wrote:

On 12/18/2012 10:00 AM, Daniel Fuchs wrote: On 12/18/12 6:29 PM, Joe Wang wrote: Hi Daniel,

The call: T provider = findServiceProvider(type) in static T find(Class type, String factoryId, ClassLoader cl, String fallbackClassName) ignored factoryId, and assumed it's the same as type.getName(). I looked back I had the same bug in my original patch. I don't think that's a bug in the new code - but rather possibly a bug in the old code ;-). The old code did not have the problem since it's reading "META-INF/services/" + factoryId directly. There's no way you can pass a property name to the ServiceLoader. True. That's why it looked a bit awkward since 'factoryId' is not passed in as did before, as in findJarServiceProvider(factoryId). The JAR Specification says: "A service provider identifies itself by placing a provider-configuration file in the resource directory META-INF/services. The file's name should consist of the fully-qualified name of the abstract service class." So I think the current code is correct in ignoring factoryId - because according to the spec the file name should be the same as the abstract class name. So the StAX API implied that factoryId could be anything, but that would violate the JAR specification. In other words, it would only work as intended if the factoryId is specified as a System Property, or in stax.properties or jaxp.properties. I would think we should return an error if factoryId != type.getName() before the call "findServiceProvider(type)".

In fact I toyed with the idea of skipping the call to findServiceProvider(type) when factoryId != type.getName(). I am not sure whether we should return an error or the skip directly to returning the default implementation.

I mean - if a user tried to load the factory specified by foo.bar, and foo.bar is not defined, shouldn't we return the default implementation? I think that's what was happening before...

But if we return the default implementation - shouldn't any Service Provider loadable through ServiceLoader take precedence?

When the parameter is a classname - then the meaning is clear. Either you can load the class or you can't. When it's a factoryId - and the doc says "it is the same as a System property" - then shouldn't the code behave as if it were a System property?

-- daniel

-Joe

-- daniel

-Joe On 12/18/2012 8:39 AM, Daniel Fuchs wrote: Hi, Thanks for the review. I updated the webrev to keep track of your suggested change. <http://cr.openjdk.java.net/~dfuchs/JDK-7169894/javax.xml.stream/webrev.01/>

-- daniel On 12/18/12 4:00 PM, Alan Bateman wrote: I looked through this installment and aside from an aside from an alignment issue at lines 101-102 in XMLEventFactory.java then it looks good to me. Also thank you again for being so careful as you work through each of these areas. -Alan

More information about the core-libs-dev mailing list