Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add FailsafeExecutionException extending FailsafeException for wrapping Throwables in sync get #356

Open
Tembrel opened this issue Jan 14, 2023 · 1 comment

Comments

@Tembrel
Copy link
Contributor

Tembrel commented Jan 14, 2023

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.

@Tembrel Tembrel changed the title Document delicate semantics of FailsafeException Add FailsafeExecutionException extending FailsafeException for wrapping Throwables in sync get Jan 23, 2023
@Tembrel
Copy link
Contributor Author

Tembrel commented Jan 23, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant