failsafe-lib/failsafe

Add FailsafeExecutionException extending FailsafeException for wrapping Throwables in sync get

Tembrel opened this issue · 1 comments

There's a subtle semantic difference between how FailsafeException is used synchronously vs. asynchronously.

With synchronous get, exceptions thrown by Failsafe policies, i.e., those that extend FailsafeException, like BulkheadFullException, can be caught directly. Application specific exceptions like IOException are wrapped as the cause of a vanilla FailsafeException. With asynchronous getAsync, both kinds of exceptions are wrapped as the cause of an ExecutionException.

There's nothing wrong with that, but it feels as though FailsafeException is filling two very different roles: On the one hand, it's a superclass for a several policy-related exceptions. On the other hand, it's a way to wrap application-specific exceptions thrown by tasks executing synchronously.

I have a dim memory of some long ago issue comments where this was explicitly acknowledged and accepted. So I don't think this warrants an API change, but it would be nice if this information were presented clearly somewhere, perhaps in a table like this:

FailsafeExecutor<R> fs R r = fs.get(() -> compute()); R r = fs.getAsync(() -> compute()).get();
BulkheadFullException
catch (BulkheadFullException bfx) {
// handle bfx directly
}
catch (ExecutionException e) {
BulkheadFullException bfx =
(BulkheadFullException) e.getCause();
// handle bfx
}
IOException
catch (FailsafeException e) {
IOException iox = (IOException) e.getCause();
// handle iox
}
catch (ExecutionException e) {
IOException iox =
(IOException) e.getCause();
// handle iox
}

I wrote a gist to make sure that my understanding of the semantics was correct.

No response to this, which is fair, since it doesn't really ask any questions or propose any action, other than better documentation.

But there's a way to make things a little nicer without any compatibility issues (I think): Add FailsafeExecutionException as a subclass of FailsafeException and use it to wrap exceptions thrown during synchronous get, instead of FailsafeException. This wouldn't break any catch (FailsafeException ...) {...} blocks, but it would allow a runtime distinction between FailsafeException as wrapper vs. as superclass.

Only two FailsafeException to FailsafeExecutionException changes would be required, that I could find, here:

if (exception instanceof RuntimeException)
throw (RuntimeException) exception;
if (exception instanceof Error)
throw (Error) exception;
throw new FailsafeException(exception);

and here:

if (e instanceof RuntimeException)
throw (RuntimeException) e;
if (e instanceof Error)
throw (Error) e;
throw new FailsafeException(e);

(Incidentally, couldn't these be consolidated?)

So I changed the issue title to reflect this proposal.