Page tree
Skip to end of metadata
Go to start of metadata

Exception Handling Improvements

        Guidelines:

  1. Do not catch the exceptions just to log and re-throw it.

    // Bad. Don't do this.
    catch (SomeException ex) {
       LOG.error("SomeException occurred.", ex);
       throw ex;
    }
     
    // Bad. Don't do this.
    catch (SomeException ex) {
       LOG.error("SomeException occurred.", ex);
       throw new AnotherException("AnotherException", ex);
    }


    Either log the exception, or throw it, but never do both. Logging and throwing results into the multiple error messages in the log file for the same problem.

  2. Declare the specific exception that your method can throw.

    // Bad. Never throw Exception
    public void myMethod() throws Exception;


    This defeats the purpose of the checked exception. Be specific about what exception the method is throwing, so that client can handle it accordingly. Also when the exceptions are used effectively, what went wrong is answered by the type of exception. The more specific the exception, the better our code answers what went wrong. 

  3. Catch the specific exception whenever possible. 

    try {
       block = readFile(dataFile);
    } catch (FileNotFoundException e) {
       // Alert user that the file is not found
    } catch (EOFException e) {
       // Alert user that the EOF is reached
    } catch (ObjectStreamException e) {
       // Alert user that the file is corrupt 
    } catch (IOException e) {
       // Alert user that some other IO error occurred
    }

    Extra catch blocks may seem coding burden, but help program respond the user with the correct message. 

  4. Throwing multiple exceptions is fine if they mean the client can take different actions. However if for multiple actions client treat them similarly, then its better to wrap those exceptions in single one and throw that from the method. 

    public void myMethod() throws MyException, AnotherException, YetAnotherException;
     
    // If above exceptions will be handled by the client similarly, then its better to wrap them in one and throw the wrapped exception
    public void anotherMethod() throws MyWrappedException;
  5. Don't loose the original exception while wrapping it with the new one.

    // Don't do this. We loose the stack trace of the original exception.
    catch (CriticalException e) {
       throw new AnotherException(e.getMessage());
    }
     
    // Instead keep the original exception as an inner exception.
    catch (CriticalException e) {
       throw new AnotherException("add more contextual information here", e);
    }
  6. Catching and returning null is almost always wrong. Instead throw exception and let the caller handle it. We should only return null in a non-exceptional use case for example: search method returns null if the person is not found in the dataset.

    // Don't do this. Instead throw an exception and let the caller handle.
    catch (NotFoundException e) {
       return null;
    }
  7. Don't throw from within the finally. Critical exception may be lost in doing so.

    try {
        try {
           o.someMethod(); // throws critical exception but is lost
        } finally {
           o.dispose(); // throws not very critical exception but is propagated
        }
      } catch (Exception e) {
    }
     
     
    // Correct way to throw critical exception 
     
    try {
        try {
           o.someMethod(); // throws critical exception
        } finally {
           try {
              o.dispose(); // throws not very critical exception
           } catch (Exception e) {
              // log the exception here
           }
        }
      } catch (Exception e) {
    }
  8. Returning from the finally block result in loosing exception information.

     try {
          throw new RuntimeException("");
       } finally {
          return;
       }
  9. Don't swallow the InterruptedException. Easiest strategy is to propagate the InterruptedException to the caller by not catching it. If you need to catch the InterruptedException for some cleanup activity, then make sure you rethrow it. Sometimes throwing InterruptedExceptionis not an option, such as when a task defined by Runnable calls an interruptible method. In this case, you can't rethrow InterruptedException, but you also do not want to do nothing. When a blocking method detects interruption and throws InterruptedException, it clears the interrupted status. If you catch InterruptedException but cannot rethrow it, you should preserve evidence that the interruption occurred so that code higher up on the call stack can learn of the interruption and respond to it if it wants to.


    // Let the caller handle the InterruptedException
    public void putValue(Value v) throws InterruptedException {
       queue.put(v); // If this throws InterruptedException, putValue propagates it to the caller without handling it
    }
     
    // Handle interrupt to perform some cleanup if required and re throw it
    public void putValue(Value v) throws InterruptedException {
       try {
         queue.put(v);  
       } catch (InterruptedException e) {
          // Perform cleanup if required
          throw e;
       }
    }
     
    // Restore the interrupted status after catching InterruptedException
    public class TaskRunner implements Runnable {
        public void run() { 
            try {
                 while (true) {
    				doSomething();
                 }
             }
             catch (InterruptedException e) { 
                 // Restore the interrupted status
                 Thread.currentThread().interrupt();
             }
        }
    }



  10. Don't rely on the getCause() method. 

    catch (MyException e) {
       if (e.getCause() instance of SomeException) {
        ...
       }
    }

    If the underlying implementation changed, you may get wrapped exception in the getCause(). Its better to use the getRootCause(), if looking for the root cause of the exception or instead, you should unwrap the causes until you find the ultimate cause of the problem that you are looking for.

  11. For exceptions due to the resource failure (e.g. DatasetService is not running), the behavior of the client is context driven. When the master process starts up, we see lot of error messages in the log file as

    ERROR [DatasetTypeManager STARTING:c.c.t.d.AbstractClientProvider@104] - Unable to discover tx service.
    
    
    ERROR [executor-3:c.c.c.g.h.NamespaceHttpHandler@67] - Internal error while listing all namespaces
    java.lang.RuntimeException: co.cask.cdap.data2.dataset2.DatasetManagementException: Cannot discover dataset service

    These messages are logged when the services which depend on the dataset service (for e.g. DefaultNamespaceEnsurer) try to access the dataset service which has not yet started. We treat them as exceptions/errors, however during master process startup, these should not be really treated as exceptions, since it is expected that the dataset service may not have started when the call to the service was made however it will get eventually started. The frequency of these messages should be minimized and the messages should be turned into the INFO.


  12. Use try-with-resources statement whenever possible. This guarantees that the resource will be closed in case of exception and also correct exception is thrown without any suppression.

  13. Logging of the exception should happen at the exception boundaries only. Exceptions are used for error handling on one side of the boundary, but they must not be permitted to leak across the boundary. One simple example of the exception boundary is the Http handlers. The exceptions generated on the handler side should not propagate to the user on the other side. 

  14. To capture the failure the detail message of an exception should contain the values of all parameter and fields that contributed to the exception. One way to ensure that the detail information is provided in the exception, design the exception class with required fields in the constructor. The message can be generated based on it.

    // In order to provide more information for the IndexOutOfBoundsException, we can design new class as
    class MyIndexOutOfBoundsException extends IndexOutOfBoundsException {
       private final int lowerBound;
       private final int upperBound;
       private final int index;
     
       public MyIndexOutOfBoundsException(int lowerBound, int upperBound, int index) {
          super("Lower Bound: " + lowerBound + ", Upper Bound: " + upperBound + ", Index: " + index);
          this.lowerBound = lowerBound;
          this.upperBound = upperBound;
          this.index = index;
       }
     
       // It is also helpful to provide the accessor methods for the failure-capture information. 
       // Client can recovered based on that. 
       public int getLowerBound() {
          return lowerBound;
       }
     
       public int getUpperBound() {
          return upperBound;
       }
     
       public int getIndex() {
          return index;
       }
    }

    This makes programmer hard to not to capture the failure-information.

  15. Higher layer should catch the lower-level exception and should throw exceptions that can be explained in terms of the higher-level abstractions.

    catch (LowerLevelException cause) {
       throw new HigherLevelException(cause);
    }
     
  16. HttpHandler should be used as a common place to handle all the exceptions for the REST api. All handlers should throw exceptions which will be handled in the HttpHandler.

  17. Document precisely the condition under which the each individual checked exception is thrown using the Javadoc @throws tag

  18. Unchecked exceptions should also be documented with the Javadoc @throws tag, but do not use throws keyword to include unchecked exceptions in the method declaration. 

  19. If exception is thrown by many methods in the class, it is acceptable to document the exception at the class level.

  20. Don't ignore exceptions. It is wise at least to log them.

  21. New code should not use the Throwables.propogate. Java 7 makes it un-necessary. Use multicatch instead. 

    // Pre Java 7
    @Override public void run() {
      try {
        delegate.run();
      } catch (Throwable t) {
        failures.increment();
        throw Throwables.propagate(t); 
      }
    }
     
    // In Java 7 we can take the advantage of the multi catch block and catch the specific exceptions that we want
    @Override public void run() {
      try {
        delegate.run();
      } catch (RuntimeException | Error e) {
        failures.increment();
        throw e; 
      }
    }
     

Approach to follow in 3.2:

Ideally newly added code should adhere to the guidelines. For the existing code, we will start the error handling improvements from the user facing components (such as UI, REST API, CLI). We plan to tackle the handler classes first and then the adapter feature.

Other enhancements:  

    • User should have the ability to change to include stack trace from UI and REST API when log calls are made. CDAP UI by default should make log calls that will not include stack trace, but messages only
      PatternLayout class controls the formatting of the log events and return the results in the String. PatternLayout can be changed to include the configured number of lines from the stack trace in the result.

    • Bug Fixes


HBase 1.1 Support:

  • Add support for HBase 1.1 in Tephra:  TEPHRA-105 - Getting issue details... STATUS
  • Add support for HBase 1.1 in CDAP:  CDAP-2556 - Getting issue details... STATUS





  • No labels

3 Comments

  1. I know I presented the idea of having a custom base exception class which binds the what, why and howToPrevent as Strings but looking at this I think it will be too cumbersome to wrap all exception in our custom exception classes. We will end up writing a lot to create such custom exceptions which have all the details. Maybe thats the price we will have to pay for helpful error messages. But we should spend some time in figuring out better/easier way to build such exceptions. For example if a user wants he can provide what message else the exception message is used by default and other things like this. 

    Also we talked about having custom exception classes for all modules like SchedulerException, AppLifeCycleException and stuff lets document that too. 

  2. The guidelines look good.

    Two comments -

    • We should add a point that talks about not overusing Throwables.propagate.
    • In addition to bad code snippets, it would be good to document the right way to do things, especially for point 5.

     

  3. Point 4. Instead of wrapper, shouldn't it be having a common ancestor Exception being defined?

    Point 7. If possible, use Closeable/AutoCloseable interface for resource cleanup related operation so that exception suppression is handled nicely. If using those interface is not possible, the following is the preferred way of doing it unless we want to if there is exception raised from o.dispose (it's actually the same as how Java handles AutoCloseable in try block).

    // If we don't want to always ignore the exception raised from o.dispose.
    try {
      Exception exception = null;
      try {
         o.someMethod(); // throws critical exception
      } catch (Exception e) {
        exception = e;
      } finally {
        if (exception != null) {
          try {
            o.dispose();
          } catch (Exception e) {
            exception.addSuppressed(e);
          }
        } else {
          o.dispose();
        }
      }
    } catch (Exception e) {
      ...
    }

    Point 21. It won't work if the run.delegate() has a throws Exception in its signature. In that case, Throwables.propagate may still be needed (Throwables.propagate won't wrap RuntimeException or Error).