Support basic Java Executor interface

[paulius-p]

Currently you can provide either ExecutorService, ScheduledExecutorService or implement custom Failsafe Scheduler.
It would be good if it would also accept java.util.concurrent.Executor interface since it is a common interface returned by other libs. My current use case is in gRPC, when splitting context for multi-threading, and gRPC methods #fixedContextExecutor return base Executor.

[whiskeysierra]

Executor is missing the notion of Future which I guess is problematic.

[jhalterman]

@whiskeysierra is right on - Failsafe wouldn't be able to return execution results from an Executor or determine if a result is a failure. You could use an Executor to run things that don't return a result though. The question then is what to do if Failsafe is asked to get something that returns a result... perhaps just not use the Executor in that case?

[paulius-p]

Yes, I understand the semantic predicament, but you could have a wrapper similarly like you have for the ExecutorService and use the CompletableFuture.supplyAsync(Supplier, Executor) to return the value?

[Tembrel]

Yes, I understand the semantic predicament, but you could have a wrapper similarly like you have for the ExecutorService and use the CompletableFuture.supplyAsync(Supplier, Executor) to return the value?

Seems reasonable to me. At the very least, couldn't the use of ExecutorService.submit in lines like this be replaced by something like:

promise.delegate = es instanceof ExecutorService ?
    ((ExecutorService) es).submit(completingCallable) :
    supplyAsync(callableToSupplier(completingCallable), es);

when es is an Executor but not an ExecutableService? I realize that I'm glossing over an important detail with the following simple implementation of callableToSupplier, but the general idea seems feasible:

static <T> Supplier<T> callableToSupplier(Callable<? extends T> callable) {
    return () -> {
	try {
	    return callable.call();
        } catch (RuntimeException ex) {
            throw ex;
        } catch (Exception ex) {
            throw new RuntimeException(ex);
        }
    };
}

[whiskeysierra]

(@Tembrel The instanceof in a catch clause can usually be written shorter and less complicated using a second catch clause.)

[Tembrel]

@whiskeysierra - oops, thanks, updated comment.

This was just a sketch meant to provoke discussion of whether it is worth offering an overload of FailsafeExecutor.with that takes an Executor. I wouldn't implement it this way IRL but wanted to suggest that it would be possible. (For one thing, I wouldn't do the instanceof ExecutorService check so late in the game; the result of that test could be determined at construction time.)

The question remains: Is this worth doing?

[jhalterman]

@paulius-p I'm curious, when creating a fixedContextExecutor, what Executor do you pass in?

@Tembrel thanks for sparking some ideas with this.

If we were to support configuring an Executor, I think it would need to be separate from and probably in addition to a Scheduler/ExecutorService. The Executor would basically just wrap Runnables/Suppliers and would amount to doing something like this:

public CheckedRunnable wrapWithExecutor(Runnable r, Executor executor) {
  return () -> {
    executor.execute(() -> {
      try {
        r.run();
      } catch (RuntimeException ex) {
        throw ex;
      } catch (Exception ex) {
        throw new RuntimeException(ex);
      }
    });
  };
}

Failsafe.with(policy).run(wrapWithExecutor(this::connect, executor));

@paulius-p Is that basically how you think this should work - simple wrapping? Maybe something like:

Failsafe.with(policy).with(executor).run(this::connect);

...but I'm not sure if this use case is too niche and the workaround too straightforward. Input welcome...

[paulius-p]

@jhalterman Yes, that's similar to what I had in mind.

For the fixedContextExecutor, in my case, I would pass in some thread pool from Executors.newFixedThreadPool(int). But in the current approach, since I cannot pass the wrapped fixedContextExecutor, I'm calling the action from within the gRPC context directly in the action supplier block:

Failsafe.with(retryPolicy)
  .with(executorService)
  .getStageAsync(() -> forkedContext.call(this::someFutureAction));

So, from verbosity perspective it's not that bad, but supporting Executor directly would reduce it slightly.

[Fadelis]

Hey, looking at it again, it's not that hard to get the Future from Executor. Java already has a way for that and it's actually used in ExecutorService impl - using FutureTask.
I've taken a quick crack at it, and it seems pretty straightforward and shouldn't break any existing features, as all ExecutorService implement Executor - Fadelis@898f961

Maybe you could have a look at this again? Thanks!

[jhalterman]

@Fadelis I pushed a PR that is similar to your suggestion: #281. Let me know what you think.

[Fadelis]

@Fadelis I pushed a PR that is similar to your suggestion: #281. Let me know what you think.

@jhalterman Yes, this should work as well 👍 Thanks for looking into this!

[jhalterman]

Closed by #281.