Null policy

[robertvazan]

Is there a way to get null/noop version of FailsafeExecutor and perhaps even of the individual policies? I would like to add a requirement to pass FailsafeExecutor to some APIs, but I have to make sure this feature can be easily disabled by submitting some kind of null/noop FailsafeExecutor that just executes everything as if the code was executed directly.

I see that Failsafe.with() will throw IllegalArgumentException if I give it an empty list of policies. I could probably find a workaround, for example by calling .handleIf(e -> false) on RetryPolicy. But is there a clean, concise solution with minimal overhead?

[hkupty]

Same issue. I would like to feature-toggle a circuit breaker for roll out, but I can't have an empty list of policies. Can't really have if (circuitBreakerEnabled) { Failsafe.with(circuitBreaker).get(this::apiMethod); } else { this.apiMethod(); } for all api methods.

[whiskeysierra]

Why don't you wrap Failsafe in a minimal interface of your own? Instead of relying on Failsafe directly everywhere, your code mainly depends on your own abstraction. And then you have a single place to do stuff like this. No need to wait for a library to provide you this.

[hkupty]

That's a good overall idea, but an investment cost I can't pay now to sell to team/pm the need of using failsafe (or any other stability framework). That's why I was going for a feature toggle solution, so they have confidence in merging.

[whiskeysierra]

That's a good overall idea, but an investment cost I can't pay now to sell to team/pm the need of using failsafe (or any other stability framework).

Honestly, I'd say it's the other way around. Binding to a 3rd-party library that you might not want to bind to or not having resilience built in both sound like something that one can't afford not to have.

[hkupty]

The problem of using a library is out of question and I'd prefer not to sidetrack the discussion to this topic.
To keep it straight:

• I need to convince and educate the team;
• to give them safety, they requested feature toggling;
• Failsafe doesn't allow for empty list of policies;

The idea is to prove the concept with one simple resource, then generalize. If I start with a big change, it'll be harder to justify.

[Tembrel]

The issue description mentions

some kind of null/noop FailsafeExecutor that just executes everything as if the code was executed directly.

This raises some questions: Would this no-op FailsafeExecutor call onComplete and onSuccess listeners? (Presumably it would never call onFailure.) Would it support asynchronous execution and configuration? Would it support ContextualSupplier/ExecutionContext? AsyncSupplier/AsyncExecution? If so, then it's hard to see how you'd beat the performance of simply adding a single always-successful policy instead of an empty list of policies.

If not, then you're asking for an implementation of FailsafeExecutor that doesn't meet its contract. One could create such an implementation and provide a wrapper that handles empty policy lists (responding more to @robertvazan here):

public final class FailsafeAllowingEmptyPolicyList {
    public static <R, P extends Policy<R>> FailsafeExecutor<R> with(List<? extends Policy<R>> policies) {
        if (policies.isEmpty())
            return new NoPoliciesFailsafeExecutor<R>();
            // Using this instead preserves FailsafeExecutor semantics on the returned value:
            // return Failsafe.with(alwaysSuccessfulPolicy());
        else
            return Failsafe.with(policies);
    }
    // ... similarly for with(P... policies)

    static class NoPoliciesFailsafeExecutor<R> extends FailsafeExecutor<R> {
        // ... pass dummy policy to super constructor
        // ... reimplement most (all?) methods, mostly trivially
    }

    Policy<R> alwaysSuccessfulPolicy() ... // e.g., handleIf(e -> false)
}

But it would be hard to convince me (and here I'm addressing both @robertvazan and @hkupty) to use a new facility that could at any point return an object that doesn't meet the contract of its interface. (There are valid cases where one might break the contract of an interface deliberately, of course, but I don't think this is good example.)

Using the commented-out return statement above instead of the incomplete FailsafeExecutor implementation would give users the option of supplying empty policy lists and still using the full FailsafeExecutor API safely and reasonably efficiently. I'm not saying I think offering this option is a good idea, just that it can be done without modifying the Failsafe code base.

Or (responding more to @hkupty here) you could use a configuration method taking a boolean argument that uses either the passed in policies or the single always-successful policy, depending on the value of that boolean. See below for how that might look.

If the only thing your users will ever do is synchronous get/run, and they don't need the full FailsafeExecutor interface, consider a simple wrapper that returns a minimal interface, as @whiskeysierra suggested, that only exposes those methods, without exposing FailsafeExecutor. For example (leaving out a few details):

interface MinimalFailsafeExecutor<R> {
    <T extends R> T get(CheckedSupplier<T> supplier) throws FailsafeException;
    void run(CheckedRunnable runnable) throws FailsafeException;
}
...
public final class MinimalFailsafe {
    
    static class Builder {
        private final boolean enabled;
        Builder(boolean enabled) { this.enabled = enabled }

        public <R, P extends Policy<R>> MinimalFailsafeExecutor<R> with(List<? extends Policy<R>> policies) {
            if (enabled) {
                FailsafeExecutor<R> fsExec = Failsafe.with(policies);
                return new MinimalFailsafeExecutor<R>() {
                    ... get(CheckedSupplier supplier) { ... fsExec.get(supplier) ... }
                    ... run(CheckedRunnable runnable) { ... fsExec.run(runnable) ... }
                };
            } else {
                return new MinimalFailsafeExecutor<R>() {
                    ... get(CheckedSupplier supplier) { ... supplier.get() ... }
                    ... run(CheckedRunnable runnable) { ... runnable.run() ... }
                };
            }
        }
    }

    public static Builder enabled(boolean enabled) { return new Builder(enabled); }

    public static <R, P extends Policy<R>> MinimalFailsafeExecutor<R> with(List<? extends Policy<R>> policies) {
        return Builder.enabled(true).with(policies);
    }
}

Then your users can write the following, and disable the Failsafe machinery with a boolean:

MinimalFailsafeExecutor<String> mfsExec = MinimalFailsafe.enabled(circuitBreakerEnabled).with(circuitBreaker);
...
String result = mfsExec.get(this::apiMethod);

That last call is nearly equivalent to this.apiMethod(), apart from the exception handling, since checked exceptions would need to be wrapped.

Later, once users are comfortable, if you decide that you need the full FailsafeExecutor interface, you can either add to this minimal interface or change MinimalInterface.with to return FailsafeExecutor instead of MinimalFailsafeExecutor, necessitating recompilation but no code change apart from imports.

[hkupty]

(...) to use a new facility that could at any point return an object that doesn't meet the contract of its interface.

I agree with you. To be honest, I was surprised to find out that an empty list of policies was not accepted (which would essentially mean no-op), but I understand there might be implementation implications. That'd be preferred over a policy that does nothing IMO.

I didn't mean to make this more than it really was: if I can configure enablement of policies through config, not a single enabled shouldn't break the application. I'm ok with the library not wanting to provide a semantically incorrect policy. This is a bit different and sounds worthy debating it...

[robertvazan]

@Tembrel Just to clarify my original post, nobody is asking for breaking the API contract. Doing so would actually reduce usefulness of the no-op implementation. The tiny performance overhead of forwarding calls through FailsafeExecutor is rarely a concern.

All we are asking for is simplicity. The alternative you proposed is not simple, at least not as simple as Failsafe.none() would be. After all, simplification of error handling code is something Failsafe is valued for.

I think your objections (i.e. performance and rendering some features useless) apply to any API that offers some sort of null implementation, for example Stream.empty() or OutputStream.nullOutputStream().

[Tembrel]

I probably confused things unnecessarily by responding to both @robertvazan and @hkupty within one long comment. So here I go with another long comment. 😉

In responding to @robertvazan's issue description, that first code snippet is complicated because it's trying to demonstrate two things:

1. that the only reasonable interpretation of a FailsafeExecutor based on an empty policy list is going to be equivalent to one using a single always-successful policy, and
2. that you can therefore roll your own trivial wrapper to get the appearance of allowing empty policy lists, if you want it that badly, without any need for a change in Failsafe behavior.

@hkupty seemed to be asking for something slightly different, a facility for bypassing Failsafe entirely based on a boolean variable, and complexity of the later code snippets is in sketching a way to achieve this without changing Failsafe.

My point was that in both cases you can get what you want without changing Failsafe, if you really want it. I was also trying to hint that maybe you shouldn't really want it. There's not much point to a null FailsafeExecutor, since it will never behave exactly like calling the target code directly, because of checked exceptions. And I think if you're going to be bypassing Failsafe with a boolean flag it's actually better (for readers of the code) to have an explicit if-else, so that users can see exactly what's going on.

Yes, it's awkward that Failsafe can't enforce the precondition that the policy list be non-empty at compile time, leading to run-time surprises. (Incidentally, why isn't it FailsafeExecutor<R> with(P firstPolicy, P... otherPolicies)?) But I think it was a mistake to let with take a list or array of policies in the first place, mainly (as I've commented on other issues) because it's too easy for users to get mixed up about which way the composition works.

OT rant (that should probably be its own issue)

Does anyone really need to build lists of policies in advance just to pass them into with(...)? I bet not. I bet most uses involve the varargs version of with using a short chain.

Wouldn't it be nicer to have FailsafeExecutor also be a builder for FailsafeExecutor? One could have wrapping(Policy p) and wrappedBy(Policy p) that would return new FailsafeExecutor instances equivalent to this but with the policy chain of this wrapping or wrapped by the given policy. Similarly for the with methods of FailsafeExecutor, and possibly the onXXX methods that currently return void.

@jhalterman has elsewhere (where?) expressed concern that it's hard to find method names that convey the composition of policies clearly, but I think wrapping and wrappedBy are pretty clear.

[brainbytes42]

If I may add my 2ct to this as well...

I ran into the Exception as well, when I was starting with the library... Simplest approach, no vararg in with()... bam.

The important question is really, how important is the ability to have an empty list / default policy for productive use...

I think it might be useful for testing in contrast to deployment or even, as mentioned above, to have some checkboxes in a configuration to toggle the Policies...

When I ran into that, I just used a quick-n-dirty solution: Retry with only one attempt. This seems to be the closest we get to the "empty" Policies...

I've put together a simple example, that illustrates the problem with the custom wrapper / interface (which in general seems very reasonable - see below).

import java.util.concurrent.ExecutionException;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.concurrent.Future;
import net.jodah.failsafe.Failsafe;
import net.jodah.failsafe.RetryPolicy;

public class FailsafeNoPolicy {

    public static void main(String[] args) throws ExecutionException, InterruptedException {


        Runnable helloTask = () -> System.out.println("Hello World @ " + Thread.currentThread());
        Runnable booomTask = () -> {throw new RuntimeException("Booom!");};

        Runnable task = helloTask;

        ExecutorService executor = Executors.newSingleThreadExecutor();

        Future<?> future;
        if(true){
            future = executor.submit(task); // just Future
        } else {

            future = Failsafe.with(new RetryPolicy<>().withMaxAttempts(1))
                                                     .with(executor)
                                                     .runAsync(task::run); // cancellable (!) Completable Future
        }
        executor.shutdown();

        future.get(); // nothing fancy with CompletionStage possible...

    }

}

The explicit distinction between Failsafe / no Failsafe is good for readability - but not for the interface. If I would create an interface / facade to wrap this distinction, I could only provide java.util.concurrent.Future instead of CompletableFuture which is way more powerful. I could use CompletableFuture's static runAsync-Methods instead of the Executor, but the returned CompletableFuture does not respond to interrupt (which is a very bad design, in my opinion, even if the reason is clear...). So to still have interruptability (which Failsafe's returned CompletableFuture provides! Yay! :-) ), We have to fall back to Executor.submit or even new Task(new FutureTask()).start, which both provide only Futures without support of CompletionStages.

Therefore, the general idea to wrap a library in a custom interface is very good, but even then I would want the CompletableFuture, which is way more labour-intensive, if it shall be cancellable...

So yes, I think an Executor, that just executes and does nothing else - like my workaround new RetryPolicy<>().withMaxAttempts(1) - would be a good thing, which might even be a reasonable default, if the Policy-List is empty...

[jhalterman]

Any preference for how to go about this? Some options:

1. FailsafeExecutor<Object> failsafe = Failsafe.with()
2. FailsafeExecutor<Object> failsafe = new FailsafeExecutor<>()

I tend to like the straightforward simplicity of option 2.

[Tembrel]

I think it's better to preserve the option of having FailsafeExecutor be an interface or abstract class, which option 2 precludes. So if I have to choose between the two, I think option 1 is better.

[jhalterman]

Interesting point @Tembrel. What do you think that might be useful for?

I wonder if there's a better option then Failsafe.with(). It's awkward to call with but not provide it with anything.

[Tembrel]

It would allow you the option of using alternative implementations based on the provided policies, rather than forcing a single implementation to serve for all.

More generally, if you don't have to have a public constructor, why make one part of the API?

How about

• providing Failsafe.none(), or something other method name that conveys the absence of any policies;
• deprecating no-arg Failsafe.with() as an alias for Failsafe.none() (or removing entirely); and
• adding Failsafe.with(Policy aPolicy, Policy... otherPolicies)?

[brainbytes42]

+1 for the first option. It is consistent with the non-empty with-calls; otherwise one wouldn'd find this option as easy, as you would need to consult another class as usual (FailsafeExecutor instead of Failsafe).

Both options seem reasonable, using an empty with(vararg) or introducing none().
Why not allowing both? So one would be able to use the explicit none or could just stick with the with but wouldn't have to bother with the split into two arguments. For the with-javadoc I'd simply state, that an empty with(vararg) is equal to none().

[Tembrel]

Providing a public constructor pins down a specific implementation without adding anything that can't be provided by a static factory method. API design best practices argue against adding dependencies unnecessarily. Once you've promised the availability of a public constructor, you can't take it back.

If FailsafeExecutor is to be used for extension by regular users, then a protected constructor would make sense, but I don't think that's in the cards right now.

[jhalterman]

@Tembrel Seeing your suggestion in another thread that with should require one policy made sense. I recently added a with method that does this: 940f6b7. Did you have in mind a reason why it's worth keeping but deprecating the no arg with() method? I realize removing it would cause a compile error but actually using it should yield a runtime exception.

[Tembrel]

No, better to fail at compile time, you're right.

…

On Fri, Aug 6, 2021, 5:36 PM Jonathan Halterman ***@***.***> wrote: @Tembrel <https://github.com/Tembrel> Seeing your suggestion in another thread that with should require one policy made sense. I recently added a with method that does this: 940f6b7 <940f6b7>. Did you have in mind a reason why it's worth keeping but deprecating the no arg with() method? I realize removing it would cause a compile error but actually using it should yield a runtime exception. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#252 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABZ5SWIYF67WBUMD73C2PTT3RIVTANCNFSM4NP4XZGA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email> .