cocoon-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Nicola Ken Barozzi <nicola...@apache.org>
Subject Re: [VOTE] Make ProcessingException extend CascadingRuntimeException
Date Sat, 17 Apr 2004 10:38:29 GMT
Ugo Cei wrote:
> Nicola Ken Barozzi wrote:
> 
>> Since you know that we have dozens of places in the code with this 
>> thing, do you care posting a dozen of them here?

Thanks for taking the time to do it. Now, let's try to analyze and see 
what is the real use of them.

> AbstractCommandLineEnvironment.java:
> 
>   } catch (ProcessingException pe) {
>     throw new CascadingIOException("ProcessingException: " + pe, pe);
>   }

This is the whole relevant code snippet with my comment below (as in 
mail replies) the commented sections:

     /**
      * Redirect the client to a new URL
      */
     public void redirect(boolean sessionmode, String newURL)
     throws IOException {

//NKB: Here it says that a redirect can throw only an IOException.
// What does this mean? It means that the caller of this can only handle
// a *generic* error on the IO, that is usually abort what it was doing.
// Hmmm...

          [...]

         // FIXME: this is a hack for the links view
         if (newURL.startsWith("cocoon:")
             && this.getView() != null
             && this.getView().equals(Constants.LINK_VIEW)) {

          [...]

             Source redirectSource = null;
             try {
                 redirectSource = this.resolveURI(newURL);
                 SourceUtil.parse( this.manager, redirectSource, ls);

//NKB: Here it says that it has to resolve an URL, but the process
//   is not necessarily something that has success.

             } catch (SourceException se) {
                 throw new CascadingIOException("SourceException: " + 
se, se);
             } catch (SAXException se) {
                 throw new CascadingIOException("SAXException: " + se, se);
             } catch (ProcessingException pe) {
                 throw new CascadingIOException("ProcessingException: " 
+ pe, pe);
             } finally {
                 this.release( redirectSource );
             }

          [...]

//NKB: Here what we are doing is saying:
// " Some finer grained exceptions can occur, but we won't do anything
//  about it. Instead I'll tell my parent that I cannot do my work."

Note a couple of things here:

1 - Reading the code I have no idea whatsoever about the 
ProcessingException: we could as easily have made it Exception that it 
has the same (lack of) meaning.

2 - The redirect takes an URL as a String, which is wrong. A method 
should do one thing, not many. If it does many, it can fail in many 
ways, but here there is only one way it can fail now: IOException.

So because of this the method should be:

     public void redirect(boolean sessionmode, String newURL)
     throws IOException, SourceException  {

or

     public void redirect(boolean sessionmode, Source newURL)
     throws IOException {

The latter is definately preferrable.

In fact, for example, SitemapSource throws a MalformedURLException, 
which should be called and thus dealt with by the caller.

3 - It redirects a SAX stream and cannot internally fail for an 
IOException... then why does it throw one?

So because of this the method should be:

     public void redirect(boolean sessionmode, String newURL)
     throws SAXException {

If it were so, we would not have try/catch.

The problem with error handling here is that the contract is wrong.


> HTMLTransformer.java (this is wicked!):
> 
>   } catch (ProcessingException e) {
>     e.printStackTrace();
>   }

     /**
      * React on endElement calls that contain a tag to be
      * tidied and run Jtidy on it, otherwise passthru.
      *
      * @see org.xml.sax.ContentHandler#endElement(java.lang.String, 
java.lang.String, java.lang.String)
      */
     public void endElement(String uri, String name, String raw)
         throws SAXException {
         if (this.tags.containsKey(name)) {
             String toBeNormalized = this.endTextRecording();
             try {
                 this.normalize(toBeNormalized);
             } catch (ProcessingException e) {

//NKB: Here we are trying to normalize a tag, but can fail.
//     What we want to do it not to fail even if the single step to
//     *try* to clean it, fails.

                 e.printStackTrace();
             }
         }
         super.endElement(uri, name, raw);
     }

Hence it should be:

             } catch (ProcessingException e) {
                logger.debug("Cleaning tag failed. Continuing... ",e);
             }


> DefaultHandlerManager.java:
> 
>   } catch (ProcessingException se) {
>     throw new ConfigurationException("Exception during configuration of 
> handler: " + name, se);
>   }

[Below I snipped all ProcessingExceptions that actually have little 
meaning. The wrong thing in particular happens because we catch a 
generic ProcessingException and narrow it, like above...]

 >   } catch (ProcessingException pe) {
 >     throw new SAXException(pe);
 >   }
 >
 > CachingSource.java (does it three times):
 >
 >   catch(ProcessingException e) {
 >     throw new CascadingIOException("Failure storing response.", e);
 >   }

[Or probably a contract mismatch instead, like above.]

[...]

> That's 23, then I stopped pasting. But considering all the catch clauses 
> for ProcessingException and its subclasses we have:
> 
> ProcessingException: 64 occurences
> ConnectionResetException: 4 occurences
> ResourceNotFoundException: 10 occurrences
> 
> But ProcessingException is just one example. Avalon's CascadingException 
>  has 14 children exception classes. For instance, 
> org.apache.avalon.framework.configuration.ConfigurationException, which 
> is heavily used by CForms for doing things like:
> 
>   throw new ConfigurationException("Convertor defined in plain attribute 
> unavailable.", e);
> 
> Now, don't tell me that distributing an incorrect configuration file is 
> not a programming error. Do contracts also cover the case of programming 
> errors? 

Yes, when there are different roles in deployment. The contract is 
between the coder and the deployer, and usually they are different.

> I suspect this should be a runtime exception. Better yet, when 
> we have Java 1.4 as a requirement, this should be:
> 
>     assert false, "Convertor defined in plain attribute unavailable.";

?

In any case, what I see is that Cocoon has gotten many contracts wrong. 
In particular it has been coded using generic Cocoonish exceptions that 
were meant to gobble up the source exceptions from the start. In fact we 
can say that what seems now as an error was deemed in the beginning as a 
feature, that was probably done to achieve that RuntimeExceptions do 
rather than to have what we have now.

I would like to note BTW that my -1 is a vote, not a veto. It is based 
on the fact that I don't believe that using RuntimeExceptions will bring 
us that cleaness we need without unwanted consequences, but this is just 
MHO and I may well again be wrong, as it happens :-)

-- 
Nicola Ken Barozzi                   nicolaken@apache.org
             - verba volant, scripta manent -
    (discussions get forgotten, just code remains)
---------------------------------------------------------------------


Mime
View raw message