search

failsafe-lib / failsafe

Fault tolerance and resilience patterns for the JVM
https://failsafe.dev
Apache License 2.0
4.16k stars 295 forks source link

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

Open Tembrel opened 1 year ago

Tembrel commented 1 year ago

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 commented 1 year ago

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:

https://github.com/failsafe-lib/failsafe/blob/0f3f66db29d801cd206079076a04d748862b4b27/core/src/main/java/dev/failsafe/SyncExecutionImpl.java#L192-L196 and here:

https://github.com/failsafe-lib/failsafe/blob/0f3f66db29d801cd206079076a04d748862b4b27/core/src/main/java/dev/failsafe/Functions.java#L272-L276 (Incidentally, couldn't these be consolidated?)

So I changed the issue title to reflect this proposal.