What is the best way to handle an ExecutionException?

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.

Attire

Just make your method throws Exception and throw all these with the same line of code. You can catch ExecutionException and just throw e.getCause -- and don't catch anything else, just let it propagate on its own. – Elvaelvah

Hi Marko, thanks for the suggestion, but I need my API to throw these 4 specific types of exception. I don't want to throw a generic Exception. – Attire

Answer 1

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.

Fatality

Marking this as the most correct answer, even though it's not the solution I was hoping for... – Attire

Indeed, I did not find any ideal solution myself, and just live with the fact that adding threaded execution fundamentally makes your exception handling more difficult - both at the code level when dealing when checked exceptions and during diagnosis because the stack has more frames and an extra "cause" that wasn't there before. – Fatality

Answer 2

Here's what I do in this situation. This accomplishes the following:

• Re-throws checked exceptions without wrapping them
• Glues together the stack traces

Code:

public <V> V waitForThingToComplete(Future<V> future) {
    boolean interrupted = false;
    try {
        while (true) {
            try {
                return future.get();
            } catch (InterruptedException e) {
                interrupted = true;
            }
        }
    } catch (ExecutionException e) {
        final Throwable cause = e.getCause();
        this.prependCurrentStackTrace(cause);
        throw this.<RuntimeException>maskException(cause);
    } catch (CancellationException e) {
        throw new RuntimeException("operation was canceled", e);
    } finally {
        if (interrupted)
            Thread.currentThread().interrupt();
    }
}

// Prepend stack frames from the current thread onto exception trace
private void prependCurrentStackTrace(Throwable t) {
    final StackTraceElement[] innerFrames = t.getStackTrace();
    final StackTraceElement[] outerFrames = new Throwable().getStackTrace();
    final StackTraceElement[] frames = new StackTraceElement[innerFrames.length + outerFrames.length];
    System.arraycopy(innerFrames, 0, frames, 0, innerFrames.length);
    frames[innerFrames.length] = new StackTraceElement(this.getClass().getName(),
      "<placeholder>", "Changed Threads", -1);
    for (int i = 1; i < outerFrames.length; i++)
        frames[innerFrames.length + i] = outerFrames[i];
    t.setStackTrace(frames);
}

// Checked exception masker
@SuppressWarnings("unchecked")
private <T extends Throwable> T maskException(Throwable t) throws T {
    throw (T)t;
}

Seems to work.

Beadle

Isn't this always going to throw unchecked exceptions? – Attire

It rethrows whatever exception was originally thrown, whether checked or not. Normally you would declare waitForThingToComplete() to throw whatever checked exceptions are thrown by the Future callback thing (if any). The caller of the method "thinks" that the callback is being invoked in the current thread even if its not. I've used this code for an AOP aspect that executes annotated methods on a different thread, making the current thread block until it completes, unbeknownst to callers of the method. – Beadle

Why "<placeholder>"? – Rectangular

That's where you would normally put the source file name. This stack trace entry is created dynamically at runtime. – Beadle

Answer 3

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.
}

Quintan

A feel very strongly that this method should throw checked exceptions. Each one should be caught and handled by the calling code. The process that I have currently maintains the exception type, it's just full of inconvenient boilerplate code. – Attire

Answer 4

I'm afraid there's no answer to your problem. Basically, you are launching a task in a different thread than the one you are in, and want to use the ExecutorService pattern to catch all the exceptions that task can throw, plus the bonus of interrupting that task after a certain amount of time. Your approach is the right one : you couldnt do that with a bare Runnable.

And this exception, that you have no information about, you want to throw it again, with a certain type : ProcessExecutionException, InterruptedException or IOException. If it's another type, you want to rethrow it as a RuntimeException (which is btw not the best solution, since you dont cover all the cases).

So you have an impendance mismatch there : a Throwable on one hand, and a known exception type on the other. The only solution you have to solve it is to do what you've done : check the type, and throw it again with a cast. It can be written differently, but will look the same in the end...

Agnomen

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.

Arri

Because I want to re-throw the original exception, not handle it at this layer in my API. I can't re-throw the exception without casting it to the specific type, and the only way that I know of the find out the specific type is with an instanceof operator. – Attire

Answer 6

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

Lobbyist

This approach sacrifices type-safety, and assumes that whoever writes the calling code will read the Javadocs to know what exception types to catch. It also will tend to catching Errors, which is a bad idea. I think it's a decent tradeoff, if we assume that the same person is writing both sides of the API. In this case, I'm writing the doSomethingWithTimeout() method which will be added to our internal development framework, and I'd really like an approach that preserves the exception types so the compiler can check it. – Attire

Answer 7

Here goes my answer. Let's suppose this code

public class Test {

    public static class Task implements Callable<Void>{

        @Override
        public Void call() throws Exception {
            throw new IOException();
        }

    }

    public static class TaskExecutor {

        private ExecutorService executor;

        public TaskExecutor() {
            this.executor = Executors.newSingleThreadExecutor();
        }

        public void executeTask(Task task) throws IOException, Throwable {
            try {
                this.executor.submit(task).get();
            } catch (ExecutionException e) {
                throw e.getCause();
            }

        }

    }



    public static void main(String[] args) {
        try {
            new TaskExecutor().executeTask(new Task());
        } catch (IOException e) {
            System.out.println("IOException");
        } catch (Throwable e) {
            System.out.println("Throwable");
        }
    }


}

IOException will be printed. I think it is an acceptable solution with the downside of throwing and catching Throwable forcefully and that the final catch can be reduced to

} catch (Throwable e) { ... }

Also, another chance is doing it in the following way

public class Test {

public static class Task implements Callable<Void>{

    private Future<Void> myFuture;

    public void execute(ExecutorService executorService) {
        this.myFuture = executorService.submit(this);
    }

    public void get() throws IOException, InterruptedException, Throwable {
        if (this.myFuture != null) {
            try {
                this.myFuture.get();
            } catch (InterruptedException e) {
                throw e;
            } catch (ExecutionException e) {
                throw e.getCause();
            }
        } else {
            throw new IllegalStateException("The task hasn't been executed yet");
        }
    }

    @Override
    public Void call() throws Exception {
        throw new IOException();
    }

}

public static void main(String[] args) {
    try {
        Task task = new Task();
        task.execute(Executors.newSingleThreadExecutor());
        task.get();
    } catch (IOException e) {
        System.out.println("IOException");
    } catch (Throwable e) {
        System.out.println("Throwable");
    }
}

}

Coelenterate

The problem with this is that if Task throws some other exception type, such as an SQLException, it will print 'throwable', when the desired result would be that the application can't compile. You need to know that the Task method could potentially throw an SQLException instead of letting the compiler check it. Once you're relying on documentation instead of the compiler, there's no benefit to checked exceptions - might as well just throw an unchecked RuntimeException subclass. – Attire

@JesseBarnum yes, I understand your point. In that case the method executeTask should add the throw of the MySQLException in oeder to maintain consistency. That's why I reformuled the proposal making the task itself "asynchronous executable". Yes, the call() method could be executed from outside and all the logic of exception handling could be avoided, but I think it is more elegant that making the "instanceof" test. – Coelenterate

Answer 8

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);
}

Seychelles

Svilen, I know that I can call ExecutionException.getCause() (that is in my existing code example). I want to re-throw the exceptions, maintaining their original type - that's where I run into trouble. – Attire

I catch Throwable because ExecutionException.getCause() returns a Throwable, not an Exception. That way, I can re-use the same variable that I'm catching instead of defining a new one. I guess that's not a big deal to add a temporary Throwable variable, but that didn't seem like much of an improvement. If you look, you'll see that I am handling the case of RuntimeExceptions and Errors by re-throwing them without modification. – Attire

I see, I guess I misunderstood your question initially. Your === UPDATE === makes it much more clear. – Seychelles

Answer 9

Here is another way to do it, though I'm not convinced that this is less clumsy or less prone to break than to do it with an instanceof check as in your question:

public static byte[] doSomethingWithTimeout(int timeout)
        throws ProcessExecutionException, InterruptedException, IOException, TimeoutException {
    ....
    try {
        ....
        return future.get(1000, TimeUnit.MILLISECONDS);
        .....
    } catch (ExecutionException e) {

        try {
            throw e.getCause();
        } catch (IOException ioe) {
            throw ioe;
        } catch (InterruptedException ie) {
            throw ie;
        } catch (ProcessExecutionException pee) {
            throw pee;
        } catch (Throwable t) {
            //Unhandled exception from Callable endups here
        }

    } catch (TimeoutException e) {
        throw e;
    } catch.....
}

Conductor

Hi Fredrik - the problem here is the catch(Throwable t) - if you assume that you don't want your method signature to be defined as throws Throwable, then you need to examine it with instanceof to rethrow RuntimeExceptions and Errors and wrap any other exception type into an unchecked exception. – Attire

I'm not sure what you're getting at but: 1. You could handle RuntimeException the same way by adding a catch (RuntimeException e) and rethrow it the same way. 2. If your code throws Error you have major issues anyway, how would you treat a VirtualMachineError? 3. Your Callable currently throws exactly those 3 checked Exceptions which are handled and rethrown by doSomethingWithTimeout(int timeout), which you mentioned was part of your API so throwing the ExecutionException's getCause() can currently only throw those checked Exceptions. What do you mean by "any other exception"? – Conductor

I don't want to try to handle Errors, but I certainly want to rethrow the Error, keeping the original type intact. That means that I need separate catch clauses for Errors before I get to catch( Throwable t). The same goes for RuntimeExceptions. That way, the only stuff that makes it to the catch( Throwable t ) clause is a checked Exception that was not in my specific rethrow list, and I can repackage that as a RuntimeException. Once you include the checks for Throwable and RuntimeException, it's not any less lines of code, although I do like that I can avoid casting the Throwable. – Attire

Answer 10

I wouldn't say I recommend this, but here is a way you can do it. It is type-safe and whoever comes to modify it after you will probably be unhappy with it.

public class ConsumerClass {

    public static byte[] doSomethingWithTimeout(int timeout)
            throws ProcessExecutionException, InterruptedException, IOException, TimeoutException {
        MyCallable callable = new MyCallable();
        ExecutorService service = Executors.newSingleThreadExecutor();
        try {
            Future<byte[]> future = service.submit(callable);
            return future.get(timeout, TimeUnit.MILLISECONDS);
        } catch (ExecutionException e) {
            throw callable.rethrow(e);
        } finally {
            service.shutdown();
        }
    }

}

// Need to subclass this new callable type to provide the Exception classes.
// This is where users of your API have to pay the price for type-safety.
public class MyCallable extends CallableWithExceptions<byte[], ProcessExecutionException, IOException> {

    public MyCallable() {
        super(ProcessExecutionException.class, IOException.class);
    }

    @Override
    public byte[] call() throws ProcessExecutionException, IOException {
        //Do some work that could throw one of these exceptions
        return null;
    }

}

// This is the generic implementation. You will need to do some more work
// if you want it to support a number of exception types other than two.
public abstract class CallableWithExceptions<V, E1 extends Exception, E2 extends Exception>
        implements Callable<V> {

    private Class<E1> e1;
    private Class<E2> e2;

    public CallableWithExceptions(Class<E1> e1, Class<E2> e2) {
        this.e1 = e1;
        this.e2 = e2;
    }

    public abstract V call() throws E1, E2;

    // This method always throws, but calling code can throw the result
    // from this method to avoid compiler errors.
    public RuntimeException rethrow(ExecutionException ee) throws E1, E2 {
        Throwable t = ee.getCause();

        if (e1.isInstance(t)) {
            throw e1.cast(t);
        } else if (e2.isInstance(t)) {
            throw e2.cast(t);
        } else if (t instanceof Error ) {
            throw (Error) t;
        } else if (t instanceof RuntimeException) {
            throw (RuntimeException) t;
        } else {
            throw new RuntimeException(t);
        }
    }

}

Subjoinder

Well, this is essentially just taking the instanceof operators and moving them to a subroutine, right? – Attire

That is one thing it does. You'll also notice that it doesn't ever have to catch Throwable. It never has to rethrow InterruptedException or TimeoutException. The generic part is cleanly factored out into a reusable class for the next time you have to solve the same problem. Finally, it is type-safe which seemed to be one of your main complaints with your original solution and the other proposed ones. Try adding an exception to MyCallable.call's throws clause. You are forced to update the future.get block to handle it. You can implement 3 and 4 exception versions to automatically handle that. – Subjoinder

Answer 11

I've found one way to solve the issue. If it's ExecutionException you can get original one by calling exception.getCause() Then you need to wrap your exception in some kind of Runtime Exception or (what is the best way for me) use @SneakyThrows annotation from project lombok (https://projectlombok.org/). I give a small piece of code example. In addition you can add a few instanceof checks before throwing an exception to be sure this is the one you're expecting.

@SneakyThrows
public <T> T submitAndGet(Callable<T> task) {
    try {
        return executor.submit(task).get(5, TimeUnit.SECONDS);
    } catch (InterruptedException | ExecutionException | TimeoutException e) {
        throw e.getCause();
    }
}

Cultch