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 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.
>>
>> --
>>
>> 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.
>
>> --
>>
>> 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.
>
>>
>> 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.
Paul.
