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) { |
catch (ExecutionException e) { |
IOException | catch (FailsafeException e) { |
catch (ExecutionException e) { |
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:
failsafe/core/src/main/java/dev/failsafe/SyncExecutionImpl.java
Lines 192 to 196 in 0f3f66d
and here:
failsafe/core/src/main/java/dev/failsafe/Functions.java
Lines 272 to 276 in 0f3f66d
(Incidentally, couldn't these be consolidated?)
So I changed the issue title to reflect this proposal.