[DISCUSS] Execution API improvements

[jhalterman]

This issue serves as a discussion place for a few proposed execution API improvements.

Removing SyncFailsafe and AsyncFailsafe

For Failsafe 2.0, we've moved to requiring Java 8 which means we have access to the ForkJoinPool's commonPool. The commonPool will allow users to perform async executions without having to configure a Scheduler. And this, I hope, will allow us to get rid of the separate SyncFailsafe and AsyncFailsafe classes (which are mostly an internal detail for users).

What this means though, is that the current API will need to change slightly to more explicitly indicate when an execution is sync vs async. Ex:

<T> T get(Callable<T> callable);

<T> Future<T> getAsync(Callable<T> callable);

While getAsync is a breaking API change, I think making async executions more explicit is a good thing in addition to hopefully allowing us to do away with the separate Sync/Async Failsafe impls.

We're also need to rename the current getAsync(AsyncExecution) method to something else, since it will collide with getAsync(ContextualCallable) and be ambiguous for lambdas. If you have any naming suggestions based on how this feature works, feel free to drop a comment below.

Saving Failsafe instances

The Failsafe class is largely just a mechanism for configuring some policies and performing executions where results are handled according to those policies. The first responsibility implies being able to save a configured Falsafe instance. Currently this looks like:

SyncFailsafe<?> failsafe = Failsafe.with(retryPolicy);
AsyncFailsafe<?> asyncFailsafe = Failsafe.with(retryPolicy).with(Scheduler.class);

What's awkward here is that we don't get a Failsafe instance, instead we get something like SyncFailsafe<T> where T represents the execution result type which we may not even know yet. Better would be:

Failsafe failsafe = Failsafe.with(fallback, retryPolicy, circuitBreaker);

...where the result type can come later and the failsafe instance can be used for lots of different executions.

Typed Results

Execution result types do matter since we want them to be unified across our event handlers. Ex:

failsafe
  .onSuccess((Boolean result) -> ...)
  .get(() -> "string") // Should not compile

The above example should not compile since onSuccess types the result as a Boolean while get types it as a String. So we sometimes want Failsafe to be typed in order to deal with this. But as mentioned above, we sometimes don't want Failsafe to be typed if we don't know what the execution result type is yet and don't care.

One idea is to offer an alternative TypedFailsafe that is there when you need it and not when you don't. Ex:

failsafe.
  .handleResult<Boolean>() // Returns TypedFailsafe<Boolean>
  .onSuccess((Boolean result) -> ...)
  .get(() -> "string") // Will not compile

This would ensure that result types are properly checked by the compiler. It's not yet clear though how well this would play with the untyped Failsafe API and how easily they'll co-exist....

We could also do something similar with Policies, that would allow us to put typed event listeners on policies for when they're needed:

retryPolicy
  .handleResult((HttpStatus status) -> ....) // returns TypedRetryPolicy<HttpStatus>
  .withMaxRetries(....

Then Failsafe can accept typed policies:

Failsafe.with(Policy<T>... policies) // Returns TypedFailsafe<T>

[whiskeysierra]

Typed Results

Instead of treating typed results as something special, why not have everything typed by default. Allowing users to use generics to specify Object/Void is easy, isn't it? Keeps the API simple, since there would be only one API, the typed version.

[jhalterman]

Instead of treating typed results as something special, why not have everything typed by default.

The reason I'm thinking is because Failsafe might receive some policy, which isn't typed for some result, then execute that policy later in a different part of the codebase. Ex:

RetryPolicy retryPolicy = new RetryPolicy().handle(ConnectException.class);
CircuitBreaker circuitBreaker = new CircuitBreaker().handle(ConnectException.class);

Failsafe failsafe = Failsafe.with(retryPolicy, circuitBreaker);

// elsewhere
failsafe
  .onSuccess((Connection result) -> ...
  .get(this::connect);

failsafe
  .onSuccess((Boolean result) -> ...
  .get(this::isConnected);

Here we're using the same Failsafe instance to perform different executions with different result types. I don't know how common this is, but I imagine it's common enough to consider.

[jhalterman]

Having an API that is gradually typed doesn't look like it will work out well, for many reasons. So I went ahead and unified the Sync and Async APIs under a new type: FailsafeExecutor. This is where all the execution logic, sync and async, will live.

[jhalterman]

This work is in master. Closing for now. Comment if anything seems really wrong :)