Fix Fallback.onFailedAttempt

Fixes #298

Author: jhalterman

CHANGELOG.md

@@ -1,4 +1,8 @@
-# Next Minor Release
+# 2.4.4
+
+### Bug Fixes
+
+- Fixed #298 - `Fallback.onFailedAttempt` not being called correctly
 
 ### API Changes
 

src/main/java/net/jodah/failsafe/Fallback.java

@@ -220,9 +220,8 @@ public boolean isAsync() {
   }
 
   /**
-   * Registers the {@code listener} to be called when an execution attempt fails. You can also use {@link
-   * #onFailure(CheckedConsumer) onFailure} to determine when the execution attempt fails <i>and</i> and the fallback
-   * result fails.
+   * Registers the {@code listener} to be called when the last execution attempt prior to the fallback failed. You can
+   * also use {@link #onFailure(CheckedConsumer) onFailure} to determine when the fallback attempt also fails.
    * <p>Note: Any exceptions that are thrown from within the {@code listener} are ignored.</p>
    */
   public Fallback<R> onFailedAttempt(CheckedConsumer<? extends ExecutionAttemptedEvent<R>> listener) {

src/main/java/net/jodah/failsafe/FallbackExecutor.java

@@ -47,6 +47,9 @@ protected Supplier<ExecutionResult> supply(Supplier<ExecutionResult> supplier, S
         return result;
 
       if (isFailure(result)) {
+        if (failedAttemptListener != null)
+          failedAttemptListener.handle(result, execution);
+
         try {
           result = policy == Fallback.VOID ?
             result.withNonResult() :
@@ -75,6 +78,9 @@ protected Supplier<CompletableFuture<ExecutionResult>> supplyAsync(
       if (!isFailure(result))
         return postExecuteAsync(result, scheduler, future);
 
+      if (failedAttemptListener != null)
+        failedAttemptListener.handle(result, execution);
+
       CompletableFuture<ExecutionResult> promise = new CompletableFuture<>();
       Callable<R> callable = () -> {
         try {
@@ -113,11 +119,4 @@ protected Supplier<CompletableFuture<ExecutionResult>> supplyAsync(
       return promise.thenCompose(ss -> postExecuteAsync(ss, scheduler, future));
     });
   }
-
-  @Override
-  protected ExecutionResult onFailure(ExecutionResult result) {
-    if (failedAttemptListener != null)
-      failedAttemptListener.handle(result, execution);
-    return result;
-  }
 }

src/test/java/net/jodah/failsafe/issues/Issue284Test.java

@@ -44,7 +44,7 @@ public void testFallbackSuccess() {
     String result = Failsafe.with(fallback).get(() -> null);
 
     assertEquals(result, "hello");
-    assertEquals(failedAttempt.get(), 0);
+    assertEquals(failedAttempt.get(), 1);
     assertTrue(success.get(), "Fallback should have been successful");
   }
 

src/test/java/net/jodah/failsafe/issues/Issue298Test.java

@@ -0,0 +1,45 @@
+package net.jodah.failsafe.issues;
+
+import net.jodah.failsafe.Failsafe;
+import net.jodah.failsafe.Fallback;
+import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.Test;
+
+import java.util.concurrent.atomic.AtomicBoolean;
+
+import static org.testng.Assert.assertFalse;
+import static org.testng.Assert.assertTrue;
+
+@Test
+public class Issue298Test {
+  AtomicBoolean failedAttemptCalled = new AtomicBoolean();
+  AtomicBoolean failureCalled = new AtomicBoolean();
+
+  Fallback<String> fallback = Fallback.<String>of(e -> "success")
+    .onFailedAttempt(e -> failedAttemptCalled.set(true))
+    .onFailure(e -> failureCalled.set(true));
+
+  @BeforeMethod
+  protected void beforeMethod() {
+    failedAttemptCalled.set(false);
+    failureCalled.set(false);
+  }
+
+  public void testSync() {
+    Failsafe.with(fallback).get(() -> {
+      throw new Exception();
+    });
+
+    assertTrue(failedAttemptCalled.get());
+    assertFalse(failureCalled.get());
+  }
+
+  public void testAsync() throws Throwable {
+    Failsafe.with(fallback).getAsync(() -> {
+      throw new Exception();
+    }).get();
+
+    assertTrue(failedAttemptCalled.get());
+    assertFalse(failureCalled.get());
+  }
+}