On 8/28/2012 1:19 AM, Paul Sandoz wrote:
Hi Joe,
On Aug 28, 2012, at 7:35 AM, Joe Wang<huizhe.w...@oracle.com> wrote:
--
datatype/FactoryFinder.java:
244 } catch (ServiceConfigurationError e) {
245 throw new DatatypeConfigurationException(e.getMessage(),
e.getCause());
You are munging the message of the exception and it's cause. Perhaps it would
be better just to pass along the SCE as the cause, that way it is better
identified that SL is being used when an error occurs.
None of the ConfigureError classes in other packages accept Error or Throwable
as did DataType (and this one is an Exception!)
So instead of making changes to the ConfigureError classes, I wrapped the
ServiceConfigurationError in jaxp configuration errors, and in this case
(Datatype), a datatype configuration exception
It should be very rare to get a ServiceConfigurationError that would indicate a
serious error in a jar configuration, basically, a non-usable implementation.
So I think we don't have to stick with the ServiceConfigurationError.
If there is an SCE it is hard for the developer to trace. Was SL used or not?
How would a developer know?
The error message is clear enough - the following for example shows the
error with or without the cause. There has been no error being reported
back ever since the start of jaxp, as you noticed, it always falls back
to the default imp, and we never heard any complaint since implementing
jaxp is just not a routing work. Adding the cause, we would have to
change the signatures of the FactoryConfigurationError, sth. we've
trying to avoid in this patch.
javax.xml.parsers.FactoryConfigurationError:
javax.xml.parsers.DocumentBuilderFactory: Provider
test.factoryfinder2.MyDocumentBuilderFactoryXX not found
at
javax.xml.parsers.FactoryFinder.findServiceProvider(FactoryFinder.java:300)
at javax.xml.parsers.FactoryFinder.find(FactoryFinder.java:258)
at
javax.xml.parsers.DocumentBuilderFactory.newInstance(DocumentBuilderFactory.java:142)
at common.Bug7169894Test.testDOMFactory(Bug7169894Test.java:85)
Caused by: java.util.ServiceConfigurationError:
javax.xml.parsers.DocumentBuilderFactory: Provider
test.factoryfinder2.MyDocumentBuilderFactoryXX not found
at java.util.ServiceLoader.fail(ServiceLoader.java:214)
at java.util.ServiceLoader.access$400(ServiceLoader.java:164)
at java.util.ServiceLoader$LazyIterator.next(ServiceLoader.java:350)
at java.util.ServiceLoader$1.next(ServiceLoader.java:421)
at javax.xml.parsers.FactoryFinder$1.run(FactoryFinder.java:286)
at java.security.AccessController.doPrivileged(Native Method)
at
javax.xml.parsers.FactoryFinder.findServiceProvider(FactoryFinder.java:283)
The cast from Throwable to Exception should be avoided, here is the code in SL:
public S next() {
if (!hasNext()) {
throw new NoSuchElementException();
}
String cn = nextName;
nextName = null;
try {
S p = service.cast(Class.forName(cn, true, loader)
.newInstance());
providers.put(cn, p);
return p;
} catch (ClassNotFoundException x) {
fail(service,
"Provider " + cn + " not found");
} catch (Throwable x) { //<--------------- this could be an
instance of Error
fail(service,
"Provider " + cn + " could not be instantiated: " + x,
x);
}
throw new Error(); // This cannot happen
}
Class.forName and newInstance can throw an instance of LinkageError. I have see
such errors from SL due to the incorrect class path settings.
The error I copied above was the only one I've seen so far.
--
parsers/DocumentBuilderFactory.java
121 public static DocumentBuilderFactory newInstance() {
122 try {
123 return (DocumentBuilderFactory)
FactoryFinder.find(DocumentBuilderFactory.class,
124 /* The default property name according to the JAXP
spec */
125 "javax.xml.parsers.DocumentBuilderFactory",
126 /* The fallback implementation class name */
127
"com.sun.org.apache.xerces.internal.jaxp.DocumentBuilderFactoryImpl");
128 } catch (FactoryConfigurationError e) {
129 throw e;
130 }
You are just re-throwing.
I did. It's not necessary technically. But since the javadoc for the method
defined it, I thought it's good to re-throw it within the method, or repeat it,
it's also making sure FactoryFinder indeed throw the error.
It's redundant code, declare FactoryConfigurationError in the method signature
if you want to declare such intent.
I did that almost automatically. But Alan thinks I'd have hard time when
it comes to compatibility review since it's a signature change.
--
SchemaFactoryFinder and XPathFactoryFinder
It seems the error behaviour has changed. AFAICT previously any exceptions
would be swallowed and null would be returned. If there is an SCE then an
exception will now be propagated. This may be OK though, just not totally sure.
Yes, it's changed. We discussed this issue before, all but Datatype swallowed
any error out of the 3rd step and they all fall back to the default impl in the
4th step. In a recent conversation with Alan, he wanted to see the error
caught, and I believe you did too. I didn't think we needed to, but I'm okay
both ways since it shall very rarely happen. Even if it does, it should have
been fixed by an impl developer long before it's out.
This is also one of those 'differences' in the api impl that kept troubling us
:)
I suspect we should be conservative and try and keep as close to the existing
behaviour as possible.
Now we're getting back to my initial patch :-) We've been stuck on this
part for quite some time. So when I said 'okay with both ways', I
really meant that I'd like to see it done. However we do it, and want it
'correct' as the word really means, no one would ever benefit from it
since no one is creating a new jaxp impl.
When using SL you are ignoring the classloader passed into the constructor. I
think you may have to pass the classloader as an argument to SL because of the
selection:
201 public static final SchemaFactory newInstance(String schemaLanguage)
202 {
203 ClassLoader cl;
204 cl = ss.getContextClassLoader();
205
206 if (cl == null) {
207 //cl = ClassLoader.getSystemClassLoader();
208 //use the current class loader
209 cl = SchemaFactory.class.getClassLoader();
210 }
Given some of the weird class loader things app servers do i am guessing
SchemaFactory may not always be loaded by the system class loader.
I did. This is the same order as in the ServiceLoader. So I thought it didn't
really make any difference. The only case where a user supplied classloader
may be used is when a factory class name is also explicitly specified.
It's not quite the same order because the SchemaFactory class may have been
loaded by a custom CL, so you need to do:
ServiceLoader.load(SchemaFactory.class, classLoader);
Normally that classLoader would be the bootstrap CL (i.e. null) and then you would be
correct, but things like app servers do some strange stuff, hence the code "cl =
SchemaFactory.class.getClassLoader();" which seems redundant on first glance and i
am guessing is there for a good reason.
Yes, it's possible technically. But it hasn't happened as far as I
know. Our AS relies on the JDK, while others, such as WebSphere may
bundle Xerces but they don't use our current API impl.
If it happens, that SchemaFactory is loaded by a custom CL, it'd result
in a convoluted jaxp instances since other parts of jaxp assume
bootstrap CL. We had this classloader-related discussion back in June,
the class loading was to be delegated to ServiceLoader on your
suggestion. So they are all rely on the SerciceLoader now.
--Joe
Paul.
