[rules-dev] Questionable Exception Handling

[Geoffrey De Smet]

1) Catching Throwable is a bad idea, because you're catching Error too.

2) Catching Exception isn't good either, because you're catching 
RuntimeException too.

Let's take a look at the code:

         try {
             object = cls.newInstance();
         } catch ( Throwable e ) {
             throw new RuntimeException("Unable to instantiate object 
for class '" + className + "'", e );
         }

Here's my class's no-arg constructor, that newInstance calls:

   MyClass() {
     if (getResource("bbb/blub.xml") == null) {
        throw new IllegalStateException("bbb/blub.xml not found");
     }
   }

That IllegalStateException shouldn't be wrapped in a RuntimeException - 
it just makes it harder to filter out the real exception.
And who knows, maybe getResource() throws a 
HibernateLazyLoadingRuntimeException.

Even "Class.forName( className );" can cause such a problem,
if they use static class constructors (which is another questionable 
design).

3) Just catch what you need to catch, no more and no less:
InstantionException etc.
Yeah, Java doesn't support:
catch (AException | BException e) {
    ...
}
So the catching code needs to be duplicated :(

With kind regards,
Geoffrey De Smet

Mark Proctor schreef:
> I'll fix in trunk soon.
> 
> Mark
> Greg Barton wrote:
>> I have to agree with Zoltan on this one.  Catching
>> Throwable is ungood.
>>
>> --- Mark Proctor <mproctor at codehaus.org> wrote:
>>
>>   
>>> If no one is agueing for Throwable, I'll update it
>>> to Exception then.
>>>
>>> Mark
>>> Zoltan Farkas wrote:
>>>     
>>>> This is my opinion also and is in line with the
>>>>       
>>> java best practices...
>>>     
>>>> Based on javadoc(see class Error and Exception):
>>>>
>>>> "The class Exception and its subclasses are a form
>>>>       
>>> of Throwable that indicates conditions that a
>>> reasonable application might want to catch. "
>>>     
>>>> "An Error is a subclass of Throwable that
>>>>       
>>> indicates serious problems that a reasonable
>>> application should not try to catch."
>>>     
>>>> In all my apps I want my application to stop on a
>>>>       
>>> unrecoverable error (like OOM). However since most
>>> libraries have some catch(Throwable) in the code, I
>>> always have to use: 
>>>     
>>>> -XX:+HeapDumpOnOutOfMemoryError
>>>>       
>>> -XX:HeapDumpPath=${DUMP_PATH}
>>> -XX:OnOutOfMemoryError="kill -9 %p"
>>>     
>>>> For me as a user of the drools engine as a 3rd
>>>>       
>>> party I would expect that Error not be "hidden"
>>> inside a RuntimeException. For me Errors are
>>> something I cannot recover from... however I might
>>> choose to continue the application on a Exception
>>> ...
>>>     
>>>> This is my opinion on this...
>>>>
>>>> thanks
>>>>
>>>> --zoly
>>>>
>>>>
>>>> ________________________________________
>>>> From: rules-dev-bounces at lists.jboss.org
>>>>       
>>> [mailto:rules-dev-bounces at lists.jboss.org] On Behalf
>>> Of Geoffrey Wiseman
>>>     
>>>> Sent: Friday, April 04, 2008 2:10 PM
>>>> To: Rules Dev List
>>>> Subject: Re: [rules-dev] Questionable Exception
>>>>       
>>> Handling
>>>     
>>>> In most cases, I'd say that Errors shouldn't even
>>>>       
>>> be caught and rethrown, personally -- something like
>>> an OOME doesn't need additional object creation, for
>>> instance.
>>>     
>>>> On Fri, Apr 4, 2008 at 1:55 PM, Mark Proctor
>>>>       
>>> <mproctor at codehaus.org> wrote:
>>>     
>>>> I don't see a problem with Throwable there? We
>>>>       
>>> rethrow again, adding in some additional
>>> information. I found that just catching Exception
>>> didn't work as you could get verification errors and
>>> other stuch things. If you look at the context of
>>> that that method is doing too, its using reflection
>>> to instantiate a class.
>>>     
>>>> Mark
>>>> Zoltan Farkas wrote: 
>>>> I was browsing through the drools source code 
>>>>  
>>>> I am not a fan of seeing catching Throwable in any
>>>>       
>>> java code ...
>>>     
>>>>  
>>>> I see this in class org.drools.util.ClassUtils:
>>>>  
>>>>     /**
>>>>      * This method will attempt to create an
>>>>       
>>> instance of the specified Class. It uses
>>>     
>>>>      * a syncrhonized HashMap to cache the
>>>>       
>>> reflection Class lookup.
>>>     
>>>>      * @param className
>>>>      * @return
>>>>      */
>>>>     public static Object instantiateObject(String
>>>>       
>>> className) {
>>>     
>>>>         Class cls = (Class)
>>>>       
>>> ClassUtils.classes.get( className );
>>>     
>>>>         if ( cls == null ) {
>>>>             try {
>>>>                 cls = Class.forName( className );
>>>>                 ClassUtils.classes.put( 
>>>>       
>>> className, cls );
>>>     
>>>>             } catch ( Throwable e ) {
>>>>                 throw new RuntimeException("Unable
>>>>       
>>> to load class '" + className + "'", e );
>>>     
>>>>             }            
>>>>         }
>>>>         
>>>>         Object object = null;
>>>>         try {
>>>>             object = cls.newInstance();           
>>>>       
>>>>         } catch ( Throwable e ) {
>>>>             throw new RuntimeException("Unable to
>>>>       
>>> instantiate object for class '" + className + "'", e
>>> );
>>>     
>>>>         }  
>>>>         return object;
>>>>     }
>>>>  
>>>> I believe this masks important errors where the
>>>>       
>>> application should not continue to run and it would
>>> be preferable to crash...
>>>     
>>>>  
>>>> --zoly
>>>>  
>>>> ________________________________________
>>>>
>>>> _______________________________________________
>>>> rules-dev mailing list
>>>> rules-dev at lists.jboss.org
>>>> https://lists.jboss.org/mailman/listinfo/rules-dev
>>>>   
>>>>
>>>>
>>>> _______________________________________________
>>>> rules-dev mailing list
>>>> rules-dev at lists.jboss.org
>>>> https://lists.jboss.org/mailman/listinfo/rules-dev
>>>>
>>>>
>>>>
>>>>   
>>>>       
>>> _______________________________________________
>>> rules-dev mailing list
>>> rules-dev at lists.jboss.org
>>> https://lists.jboss.org/mailman/listinfo/rules-dev
>>>
>>>     
>>
>>
>>
>>       ____________________________________________________________________________________
>> You rock. That's why Blockbuster's offering you one month of Blockbuster Total Access, No Cost.  
>> http://tc.deals.yahoo.com/tc/blockbuster/text5.com
>> _______________________________________________
>> rules-dev mailing list
>> rules-dev at lists.jboss.org
>> https://lists.jboss.org/mailman/listinfo/rules-dev
>>
>>   
> 
> 
> ------------------------------------------------------------------------
> 
> _______________________________________________
> rules-dev mailing list
> rules-dev at lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/rules-dev