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

handleresultif and fallback with exception -- possible? #354

Closed adrian-herscu closed 1 year ago

adrian-herscu commented 1 year ago

In a testing scenario, some REST endpoint sporadically returns a non-200 status. In case that it eventually returns a 200, then the test should continue. In case that it keeps returning a non-200, then the test should fail with some exception.

Tried the following:

        try {
            Failsafe.with(
                new RetryPolicy<Integer>()
                    .withMaxRetries(3)
                    .handleResultIf(result -> result == 0)
                    .onRetry(event -> log.info("Retrying...")),
                // does not work :(
                Fallback.of(() -> { throw new SOME_EXCEPTION(); }))
                .get(() -> 0);

            fail("an exception should have been thrown");
        } catch (final RuntimeException t) {
            log.info("failed with {}", t.toString());
        }

also tried:

(Policy<Integer>) execution -> {
                    throw new NullPointerException();
                }

instead of the Fallback, but then no retries occur :(

Any suggestions?

Tembrel commented 1 year ago

Several things here:

With that in mind, here's a unit test that shows both success and failure:

    Fallback<Integer> FALLBACK =
        Fallback.<Integer>builderOfException(e -> new RuntimeException("req fail"))
            .handleResultIf(this::badStatus)
            .build();

    RetryPolicy<Integer> RETRY =
        RetryPolicy.<Integer>builder()
            .withMaxRetries(3)
            .handleResultIf(this::badStatus)
            .onFailedAttempt(e ->
                System.out.printf("Attempt #%d failed%n", e.getAttemptCount()))
            .build();

    FailsafeExecutor<Integer> EXEC =
        Failsafe.with(FALLBACK).compose(RETRY);

    @Test
    public void successfulCase() {
        int result = runWithStatuses(new int[] { 404, 200 });
        assertEquals(200, result);
    }

    @Test(expected = RuntimeException.class)
    public void unsuccessfulCase() {
        runWithStatuses(new int[] { 404, 404, 404, 404 });
        fail("should have failed");
    }

    int runWithStatuses(int[] statuses) {
        AtomicInteger i = new AtomicInteger(0);
        return (int) EXEC.get(() -> statuses[i.getAndIncrement()]);
    }

    boolean badStatus(int status) {
        return status != 200;
    }

Note the use of the badStatus method to make it clear that the fallback and retry policy have the same notion of what kinds of situations should be handled.

And note the use of compose to make it clear which policy is outermost.

jhalterman commented 1 year ago

@Tembrel's response is spot on. The only thing I'd add is a few links to docs that describe some of the things he mentioned:

adrian-herscu commented 1 year ago

Thanks for the detailed explanation :)

Since I am still with version 2.x.x, here is the complete working configuration:

Failsafe.with(
                Fallback
                    .<Integer> ofException(
                        e -> new RuntimeException(e.getLastResult().toString()))
                    .handleResultIf(result -> result == 0),
                new RetryPolicy<Integer>()
                    .withMaxRetries(3)
                    .handleResultIf(result -> result == 0)
                    .onRetry(event -> log.info("Retrying...")))
adrian-herscu commented 1 year ago

BTW... Is it possible somehow to git rid of the duplicate .handleResultIf(result -> result == 0) ?

Tembrel commented 1 year ago

BTW... Is it possible somehow to git rid of the duplicate .handleResultIf(result -> result == 0) ?

Not if you want the RetryPolicy and the Fallback to share the same notion of what constitutes a "bad" result, and you very much do, in this case.

I think a best practice here (illustrated above as well) is to encapsulate that predicate with a suggestively named method in the class where the policies are defined. Then you can make the duplication explicit:

Fallback fb = Fallback.builder(...)
    .handleResultIf(this::isBadResult)
    .build();

RetryPolicy rp = RetryPolicy.builder()
    .handleResultIf(this::isBadResult)
    .build();

This explicit repetitiveness might seem onerous, but consider that there are situations where different policies in a composition need different handling. For example, a retry policy that exceeds its limits returns the result or exception from its last attempt. In a slightly different setting from the one you describe, it might be desirable to return the last result, e.g., a non-200 status code, but still provide a fallback for a particular exception:

Fallback<Integer> fb = Fallback.<Integer>builder(504)
    .handle(NetworkException.class)
    .build();

RetryPolicy<Integer> rp = RetryPolicy.<Integer>builder()
    .withMaxAttempts(MAX_ATTEMPTS)
    .handleResultIf(this::isBadStatusCode)
    .build();

FailsafeExecutor<Integer> exec = Failsafe.with(fb).compose(rp);

Note that handleResultIf doesn't remove the default handling for exceptions.

The FailsafeExecutor defined here will return the result of the first attempt that doesn't have a bad status code or the last result if all the attempts have bad status codes (or throw exceptions). If a NetworkException is thrown by the last attempt, the fallback intercepts it and replaces it with a 504 result. Otherwise, an exception thrown on the last attempt is propagated and ultimately thrown wrapped by a FailsafeException.

Here's a gist that tests that code.

jhalterman commented 1 year ago

Closing for now. Feel free to re-open if the above didn't answer your question.