On 6/21/2012 2:35 AM, Paul Sandoz wrote:
Hi Joe,
It looks a good start.
Thanks for the detailed review!
There is duplication in the 6 factory finding classes, can some of it
be consolidated in one shared factory helper class?
The duplication of FactoryFinder and SecuritySupport classes was
determined by the security team at the time in that these classes should
not be made public. The same existed in the RI. In a recent security
change, we removed 52 duplicated classes and used package restriction
instead. I could consult security if we want to do that.
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?
ConfigurationError is internally defined. It's a contract between the
Factory and FactoryFinder classes. It seems to me it was a result of
sharing the FactoryFinders and different exception types defined in the
spec for different factories.
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.
JAXP is an supported endorsed technology. It can be placed in the
endorsed directory or bootclasspath to replace the default impl. When a
3rd party impl is on the class path however, it should take preference
over the default impl, thus the check for the one loaded by the context
class loader. The ServiceLoader however, is doing a lot more than the
original META-INF/services process. It's finding providers installed "in
the form of extensions" and placed on the class path, it therefore
always loads JAXP whether it's in the endorsed directory or
bootclasspath. That's the reason I cached it as default impl intended to
be used to replace that in the JDK. When a 3rd party impl is on the
classpath, we still need to let it take preference.
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.
Yes, that's when I observed too that the ServiceLoader is doing more as
I described above.
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.
As I mentioned above, the first found can be jaxp RI.
--
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.
That would be nice. One other thing I noticed was that .load returns an
instance. There's a situation where a static method or constructor with
a parameter instead of the default constructor may be called to initiate
the provider.
-Joe
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/
<http://cr.openjdk.java.net/%7Ejoehw/jdk8/7169894/webrev/>
Thanks,
Joe
