Hi Joe,
It looks a good start.
There is duplication in the 6 factory finding classes, can some of it be
consolidated in one shared factory helper class?
Taking javax.xml.datatyoe.FactoryFinder as an example:
When iterating over the service instances you are catching ConfigurationError
264 ServiceLoader loader = ServiceLoader.load(serviceClass, cl);
265 final Iterator providers = loader.iterator();
266 while (providers.hasNext()) {
267 try {
268 Object provider = providers.next();
269 if
(provider.getClass().getName().contains(fallbackClassName)) {
270 defaultProvider = provider;
271 } else {
272 return provider;
273 }
274 } catch (ConfigurationError e) {
275 // This can happen because of class loader mismatch or any
other reason.
276 // log and continue to next one
277 if (debug) {
278 dPrint("The provider can not be instantiated due to: "
+ e + ".");
279 }
280 }
281 }
Did you mean ServiceConfigurationError?
IIUC the previous code parsed a META-INF/services file and picked the first
entry if present and attempt to instantiate that.
I gather the approach you want to achieve with ServiceLoader is to "keep on
trucking". In addition if nothing but an instance of the default service
provider class is obtained then use that. From what i can tell the
"fallbackClassName" parameter is a fully qualified class name so you need to do
getName().equals(fallbackClassName).
An alternative approach is to always assume that the default service provider
class is never registered in META-INF/services or in module declaration. It
should simplify things.
You are first trying to use ServiceLoader with the thread context class loader,
then if that fails using the system class loader (since
FactoryFinder.class.getCLassLoader() == null).
234 private static Object findServiceProvider(String factoryId, String
fallbackClassName)
235 throws ConfigurationError
236 {
237 final Class<?> serviceClass;
238 try {
239 serviceClass = Class.forName(factoryId);
240 } catch (ClassNotFoundException e) {
241 throw new ConfigurationError("Unable to load " + factoryId, e);
242 }
243
244 Object provider = null;
245
246 // First try the Context ClassLoader
247 ClassLoader cl = ss.getContextClassLoader();
248 if (cl != null) {
249 provider = findProvider(serviceClass, cl, fallbackClassName);
250 if (provider != null) return provider;
251 }
252
253 // If no provider found then try the current ClassLoader
254 provider = findProvider(serviceClass,
FactoryFinder.class.getClassLoader(), fallbackClassName);
255 if (provider != null) return provider;
256
257 return null;
258 }
I am wondering if we can just get away with using
ServiceLoader.load(serviceClass), possibly not given the current implemented
(but not documented) behavior. In any case we should document the class loaders
being used, and in what order, with ServiceLoader.
I am not so sure about the "keep on trucking" approach when iterating over
service instances. The service mechanism is being used to register a factory
class that is a service provider class. If the first item in the service
instance iterator cannot be instantiated it signals a configuration error.
--
I think the use of ServiceLoader by JAXP is really good input to improving
ServiceLoader e.g.:
ServiceLoader.withDefault(defaultServiceProviderClass).load(serviceInterface);
or
MyService serviceInstance =
ServiceLoader.withDefault(defaultServiceProviderClass).
first(serviceInterfaceClass);
You could emulate this with your own shared factory helper class.
Paul.
On Jun 21, 2012, at 9:55 AM, Joe Wang wrote:
> Hi,
>
> This is a patch to replace the manual process in the 3rd step of the JAXP
> implementation resolution mechanism with ServiceLoader. Please see
> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7169894 for more details
> about the issue.
>
> Note that FactoryFinder is duplicated for each JAXP subpackage. The ones in
> the following packages are the same:
> /jaxp-api/src/javax/xml/datatype/FactoryFinder.java
> /jaxp-api/src/javax/xml/parsers/FactoryFinder.java
> /jaxp-api/src/javax/xml/stream/FactoryFinder.java
> /jaxp-api/src/javax/xml/transform/FactoryFinder.java
>
> The following two are similar except that they perform Schema Language or
> Object Model support check:
> /jaxp-api/src/javax/xml/validation/SchemaFactoryFinder.java
> /jaxp-api/src/javax/xml/xpath/XPathFactoryFinder.java
>
> It's a bit rush since I have only had time to test regular JDK using JDK
> 1.6.0_27. Further test on jigsaw is needed.
> All jaxp unit/SQE tests are passed. But TCK test is still running. So please
> take this as an initial review.
>
> webrev:
> http://cr.openjdk.java.net/~joehw/jdk8/7169894/webrev/
>
> Thanks,
> Joe
>
>
