XMLConfigurator.createClassInstance() should catch Throwable ?

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

XMLConfigurator.createClassInstance() should catch Throwable ?

Tom Zeller-3
TL;DR I think that
org.opensaml.core.xml.config.XMLConfigurator.createClassInstance()
should catch/handle Throwable in addition to Exception.

The tests for IdP 3.3.x were failing because I inadvertently had mixed
versions of OpenSAML on the test (Maven Surefire) classpath,
opensaml-api-* was 3.3.1 but opensaml-impl-* was 3.4.0-SNAPSHOT. It
was IDP-1217 that exposed the version mixmatch.

The tests were failing with :

java.lang.NoClassDefFoundError:
org/opensaml/saml/ext/reqattr/RequestedAttributes
Caused by: java.lang.ClassNotFoundException:
org.opensaml.saml.ext.reqattr.RequestedAttributes

when calling InitializationService.initialize();

Catching Throwable won't fix anything, but it should lead to a clearer
error message, I would think.

Tom
--
To unsubscribe from this list send an email to [hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: XMLConfigurator.createClassInstance() should catch Throwable ?

Brent Putman



On 5/31/18 5:04 PM, Tom Zeller wrote:
TL;DR I think that
org.opensaml.core.xml.config.XMLConfigurator.createClassInstance()
should catch/handle Throwable in addition to Exception.



I think that's fine to do. Probably also need to widen the ctor args of XMLConfigurationException to take a Throwable instead of Exception, so as not to have nasty conditional logic in the catch.

I think it does lead though to the more philosophical question in Java of when/where we should care about this case in general.  Any code anywhere can theoretically throw an unchecked java.lang.Error.  But we obviously aren't going to generally catch Throwable *everywhere*, as opposed to specific types of checked exceptions.  And we don't generally catch java.lang.Exception to cover unchecked java.lang.RuntimeException, for the same reasons.  Both of those are generally fatal (which is why they are unchecked), so it begs the question when is it appropriate to handle explicitly.  If there's a specific case we've actually encountered then I guess that's a good argument - for better logging, etc.  But there's probably umpteen other similar cases we don't (yet).

--
To unsubscribe from this list send an email to [hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: XMLConfigurator.createClassInstance() should catch Throwable ?

Tom Zeller-3
> If there's a specific case we've actually encountered then I
> guess that's a good argument - for better logging, etc.  But there's
> probably umpteen other similar cases we don't (yet).

Yes, I think I had the same/similar thought.

I guess because this particular use/error-case is in a "core" package
and involves a classloader it seems worth the effort to catch/handle
Throwable to help troubleshoot classpath issues.

But I agree, we could certainly just doc this one and move on.

Tom
--
To unsubscribe from this list send an email to [hidden email]