Exception Handling Improvements
- Sagar Kapare
- Vinisha Shah
- Nitin Motgi
- Rohit Sinha
Exception Handling Improvements
Guidelines:
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.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.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.
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;
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); }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; }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) { }Returning from the finally block result in loosing exception information.
try { throw new RuntimeException(""); } finally { return; }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(); } } }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.
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.
- 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.
- 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.
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.
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); }- 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.
- Document precisely the condition under which the each individual checked exception is thrown using the Javadoc @throws tag
- 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.
- If exception is thrown by many methods in the class, it is acceptable to document the exception at the class level.
- Don't ignore exceptions. It is wise at least to log them.
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
- 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
- Error rendering macro 'jira' : Unable to locate Jira server for this macro. It may be due to Application Link configuration.
HBase 1.1 Support:
- Add support for HBase 1.1 in Tephra:
- Add support for HBase 1.1 in CDAP: