What is the best way to handle an ExecutionExcepti

Question

I have a method that performs some task with a timeout. I use the ExecutorServer.submit() to get a Future object, and then I call future.get() with a timeout. This is working fine, but my question is the best way to handle checked exceptions that can be thrown by my task. The following code works, and preserves the checked exceptions, but it seems extremely clumsy and prone to break if the list of checked exceptions in the method signature changes.

Any suggestions on how to fix this? I need to target Java 5, but I'd also be curious to know if there are good solutions in newer versions of Java.

public static byte[] doSomethingWithTimeout( int timeout ) throws ProcessExecutionException, InterruptedException, IOException, TimeoutException {

    Callable<byte[]> callable = new Callable<byte[]>() {
        public byte[] call() throws IOException, InterruptedException, ProcessExecutionException {
            //Do some work that could throw one of these exceptions
            return null;
        }
    };

    try {
        ExecutorService service = Executors.newSingleThreadExecutor();
        try {
            Future<byte[]> future = service.submit( callable );
            return future.get( timeout, TimeUnit.MILLISECONDS );
        } finally {
            service.shutdown();
        }
    } catch( Throwable t ) { //Exception handling of nested exceptions is painfully clumsy in Java
        if( t instanceof ExecutionException ) {
            t = t.getCause();
        }
        if( t instanceof ProcessExecutionException ) {
            throw (ProcessExecutionException)t;
        } else if( t instanceof InterruptedException ) {
            throw (InterruptedException)t;
        } else if( t instanceof IOException ) {
            throw (IOException)t;
        } else if( t instanceof TimeoutException ) {
            throw (TimeoutException)t;
        } else if( t instanceof Error ) {
            throw (Error)t;
        } else if( t instanceof RuntimeException) {
            throw (RuntimeException)t;
        } else {
            throw new RuntimeException( t );
        }
    }
}

=== UPDATE ===

Many people posted responses that recommended either 1) re-throwing as a general exception, or 2) re-throw as an unchecked exception. I don't want to do either of these, because these exception types (ProcessExecutionException, InterruptedException, IOException, TimeoutException) are important - they will each be handled differently by the calling processed. If I were not needing a timeout feature, then I would want my method to throw these 4 specific exception types (well, except for TimeoutException). I don't think that adding a timeout feature should change my method signature to throw a generic Exception type.

Answer 1

Here are couple of interesting information for checked and against Checked Exceptions. Brian Goetz discussion and an argument of against checked exceptions from Eckel Discussion. But I did not know if you have already implemented and given a thought about the checked exception refactor that is discussed by Joshua in this book.

According the Effective Java pearls, one of the preferred method of handling Checked exceptions is to turn a checked exception into an Un-Checked Exception. So for example,

try{
obj.someAction()
}catch(CheckedException excep){
}

change this implementation to

if(obj.canThisOperationBeperformed){
obj.someAction()
}else{
// Handle the required Exception.
}

Answer 2

The javadoc of java.util.concurrent.Future.get() states the following. Then why not just catch ExecutionException (and Cancellation and Interrupted as declared by the java.util.concurrent.Future.get()) method?

...
Throws:

CancellationException - if the computation was cancelled

ExecutionException - if the computation threw an exception

InterruptedException - if the current thread was interrupted while waiting

So basically you can throw whatever exception within your callable and just catch ExecutionException. Then ExecutionException.getCause() will hold the actual exception your callable threw as stated in the javadoc. This way you are shielded from method signature changes related to checked exception declaration.

By the way you should never catch Throwable, as this would catch also RuntimeExceptions and Errors. Catching Exception is a little bit better but still not recommended, as it will catch RuntimeExceptions.

Something like:

               try {

                    MyResult result = myFutureTask.get();
                } catch (ExecutionException e) {
                    if (errorHandler != null) {
                        errorHandler.handleExecutionException(e);
                    }
                    logger.error(e);
                } catch (CancellationException e) {
                    if (errorHandler != null) {
                        errorHandler.handleCancelationException(e);
                    }
                    logger.error(e);

                } catch (InterruptedException e) {
                    if (errorHandler != null) {
                        errorHandler.handleInterruptedException(e);
                    }
                    logger.error(e);
                }

Answer 3

I've looked at this problem in depth, and it's a mess. There is no easy answer in Java 5, nor in 6 or 7. In addition to the clumsiness, verbosity and fragility that you point out, your solution actually has the problem that the ExecutionException that you are stripping off when you call getCause() actually contains most of the important stack trace information!

That is, all the stack information of the thread executing the method in the code you presented is only in the ExcecutionException, and not in the nested causes, which only cover frames starting at call() in the Callable. That is, your doSomethingWithTimeout method won't even appear in the stack traces of the exceptions you are throwing here! You'll only get the disembodied stack from the executor. This is because the ExecutionException is the only one that was created on the calling thread (see FutureTask.get()).

The only solution I know is complicated. A lot of the problem originates with the liberal exception specification of Callable - throws Exception. You can define new variants of Callable which specify exactly which exceptions they throw, such as:

public interface Callable1<T,X extends Exception> extends Callable<T> {

    @Override
    T call() throws X; 
}

This allows methods which executes callables to have a more precise throws clause. If you want to support signatures with up to N exceptions, you'll need N variants of this interface, unfortunately.

Now you can write a wrapper around the JDK Executor which takes the enhanced Callable, and returns an enhanced Future, something like guava's CheckedFuture. The checked exception type(s) are propagated at compile time from the creation and type of the ExecutorService, to the returned Futures, and end up on the getChecked method on the future.

That's how you thread the compile-time type safety through. This means that rather than calling:

Future.get() throws InterruptedException, ExecutionException;

You can call:

CheckedFuture.getChecked() throws InterruptedException, ProcessExecutionException, IOException

So the unwrapping problem is avoided - your method immediately throws the exceptions of the required type and they are available and checked at compile time.

Inside getChecked, however you still need to solve the "missing cause" unwrapping problem described above. You can do this by stitching the current stack (of the calling thread) onto the stack of the thrown exception. This a stretching the usual use of a stack trace in Java, since a single stack stretches across threads, but it works and is easy to understand once you know what is going on.

Another option is to create another exception of the same thing as the one being thrown, and set the original as the cause of the new one. You'll get the full stack trace, and the cause relationship will be the same as how it works with ExecutionException - but you'll have the right type of exception. You'll need to use reflection, however, and is not guaranteed to work, e.g., for objects with no constructor having the usual parameters.

Answer 4

In the calling class, catch the Throwable last. For instance,

try{
    doSomethingWithTimeout(i);
}
catch(InterruptedException e){
    // do something
}
catch(IOException e){
    // do something
} 
catch(TimeoutException e){
    // do something
}
catch(ExecutionException e){
    // do something
}
catch(Throwable t){
    // do something
}

And the content of doSomethingWithTimeout(int timeout) should look like this,

.
.
.
ExecutorService service = Executors.newSingleThreadExecutor();
try {
    Future<byte[]> future = service.submit( callable );
    return future.get( timeout, TimeUnit.MILLISECONDS );
} 
catch(Throwable t){
    throw t;
}
finally{
    service.shutdown();
}

And it's method signature should look like,

doSomethingWithTimeout(int timeout) throws Throwable

Answer 5

I'm not sure why you have the if/else block in the catch and instanceof, I think you can do what you want with:-

catch( ProcessExecutionException ex )
{
   // handle ProcessExecutionException
}
catch( InterruptException ex )
{
   // handler InterruptException*
}

One thing to consider, to reduce clutter, is to catch the exception inside your callable method and re-throw as your own domain/package specific exception or exceptions. How many exceptions you need to create would largely depend on how your calling code will respond to the exception.