3 Replies Latest reply on Jul 13, 2005 8:45 AM by acoliver

    Exception Handling

      Hi,

      I think we need a consistant approach to exception handling. We have a mix of no exceptions (swallowing and returning false/null), checked and unchecked exceptions. I have been think about this for a while, but today an article cropped up on JavaWorld that sums up my preferred approach.

      http://www.javaworld.com/javaworld/jw-07-2005/jw-0711-exception.html

      I think the 3 guidelines specfied at the beginning of the article would work quite well. How should these be applied to JBoss-Mail? All module interfaces should be configured to only throw a MailException (or a sub-class). This would only apply to methods exposed to other modules. Internal to a module any type of exception may be declared. MailExceptions should be thrown in the case of an application error. E.g. User does not exist. A rule of thumb would be that you should throw a MailException in the case of an application check failing e.g.

      User user = Hibernate.getUser(); // Some system find method
      if (user != null) {
       return getFolderForUser(user);
      }
      else {
       throw new UserNotFoundException("......"); // extends MailException
      }


      Calling code may attempt handle the exception if appropriate.

      Folder f;
      try {
       f = getFolder();
      }
      catch (UserNotFoundException e) {
       f = createFolderForUser();
      }


      If the calling decides not to handle the exception it should be allowed to cascade up.

      try {
       f = getFolder();
      } // no catch
      finally {
       // Do some clean up if necessary.
      }


      There is a danger of this becoming programming by execption which is not good. However the cases where exception are simply allowed to cascade up the stack will outweigh the ones where it is caught and handled, i.e. the exceptional cases (pun intended).

      If an exception is thrown which can not be handled locally and it is not possible rethrow the same exception then a wrapped MailSystemException (sub-class of RuntimeException) should be thrown.

      try {
       doStuff();
      }
      catch (SQLException e) {
       throw new MailSystemException("...", e);
      }


      Exceptions should be handled at the entry point into the mail server. This includes the protocol implementations and delivery MDBs. Each entry point will contain a static table that maps internal mail exceptions to error responses appropriate to the particular entry (as defined by POP, IMAP etc.)


      try {
       // do some stuff.
      }
      catch (MailException e) {
       Response r = processException(e);
      }
      catch (RuntimeException e) {
       Response r = processUnknownException(e);
      }


      The motivation for this is there are a number of interfaces that don't throw exceptions on error, which causes problems when introducing AOP transactions that use exceptions to trigger a rollback when required. Also it would be cleaner to have a common approach to handling errors.

      I am interested in hearing peoples' thoughts on the subject.

      Mike.

        • 1. Re: Exception Handling
          acoliver

          overall I think tis good but i think all exceptions should be runtime exceptions. If you choose to catch it locally, then fine.

          the state/conversational nature of the protocol doesn't always allow them to be caught at the entry point and I'm unsure that this will make the code cleaner.

          I think that we have 4 types:

          1. Stream killers (so don't bother telling the user)
          2. It didn't taste good/stream ok (so tell the user)
          3. detached exceptions (couldn't deliever so maybe tell the user by sending him a "couldn't deliver" email preferrably with more detail than we have today!!!)
          4. Panics (SQL Exception is a BAD BAD error, tell someone, bad stuff is happening, we've lost the DB, we have deadlocks, fix us!!!) -- we may want to do service unavailable rather than accept messages that won't be delivered.

          -Andy

          • 2. Re: Exception Handling

            I think 2 & 3 are the same (MDBs are entry points in their own special way :-). Sending an 'couldn't deliver' email is exception handling rather than an a type of exception. I see these as 2 orthoginal concepts. I am happy to use RuntimeExceptions for all exceptions, but would like to have seperate base exceptions to distingush different types.

            Looking closer at the protocols I see what you mean about the conversation state stuff. However if we choose to handle the error locally then we should make sure something useful is returned to the caller. Either return a valid object or throw an exception. We shouldn't swallow the exception, log it and return null. There are few cases where this happens and the protocol gets into an unknown state, throws a null pointer exception and falls over in a heap. Also people tend to forget to check for nulls (I know I do).

            Cleanliness will hopefully be a result of a consistent approach.

            • 3. Re: Exception Handling
              acoliver

              Base classes are good.

              2/3 to me are different to me, mentally. Meaning if you gave me bad stuff, I tell you to give me good stuff (authentication as an example). Meaning if its a "other side is busted" its one thing and if I have no conversation with the user anymore, I have to do more work for the error message (create status and stick it in his mailbox potentially).

              -Andy