withExecutor causes Failsafe not to retry on exceptions in some cases

[korri123]

Hey, thank you for this great library! I've got an issue to report however.

@Test
void failsafeFail() throws InterruptedException {
    AtomicInteger counter = new AtomicInteger(0);
    Executor executor = Executors.newSingleThreadExecutor();
    Failsafe.with(RetryPolicy.builder()
                            .handle(RuntimeException.class)
                            .withMaxAttempts(2)
                            .build())
            .with(executor)
            .runAsync(() -> {
                if (counter.incrementAndGet() == 1)
                    throw new RuntimeException();
            });
    Thread.sleep(100);
    assertEquals(counter.get(), 2);
}

If you comment out .with(executor) the test case will work. When the case fails the method is run from Functions.java#withExecutor(ContextualSupplier<R, T> supplier, Executor executor) which calls handleExecutableThrowable which just throws the exception again. With the line commented out it uses a different code path that actually handles the exception.

[Tembrel]

Aside: The overloading introduced by #221 isn't helping here. If you replace the line:

    Executor executor = Executors.newSingleThreadExecutor();

with

    ExecutorService executor = Executors.newSingleThreadExecutor();

the test passes as expected. Would it be terrible to use a longer name for the wrapping case, e.g., withExecutor(Executor)? I think a big ugly name is useful to warn users that they are getting special treatment.

[jhalterman]

Thanks for filing this. I agree @Tembrel, a more explicit name wouldn't be bad. Thinking of other options though, since the intent here is to provide an ExecutorService, I wonder if with(Executor) should do an instanceof check and re-route ExecutorService instances to with(ExecutorService). At least this could be a short term fix in lieu of more explicit config method names. @Tembrel what do you think?

Edit: A similar check for with(ExecutorService) might make sense as well, to make sure we're not actually working with a ScheduledExecutorService, in which case we don't need to bother wrapping the executorService with DelegatingScheduler.

[Tembrel]

How about adding those instanceof checks (for ExecutorService and ScheduledExecutorService) and also provide explicit config methods withExecutor(Executor), withExecutorService(ExecutorService), and withScheduler (the latter could continue to be overloaded by ScheduledExecutorService and Scheduler)? Existing code using with would do the right thing by avoiding unnecessary wrapping, and new code could be explicit about intent, allowing users to request wrapping even if unneeded. (For example, one might want to preserve the pre-instanceof-check semantics of with.)

Explicit names are nice for those who read others' code; they can skip that momentary confusion that occurs when encountering a line with no type information:

    .with(CONNECT_POOL)

You can argue that CONNECT_POOL isn't a great name, but it's not an unrealistic one. I'd rather see this in that case:

    .withExecutorService(CONNECT_POOL)

Maybe an explicit name like withPlainExecutor(Executor) would keep people from using that method accidentally.

[jhalterman]

3.0.2 is released with a fix for this regression.

[korri123]

Amazing, thank you!