From 821b7ff5ab7a4d8f1629c01bc37b518f66524ea0 Mon Sep 17 00:00:00 2001
From: Jonathan Halterman <jhalterman@gmail.com>
Date: Fri, 17 Sep 2021 19:27:19 -0700
Subject: [PATCH] Fix Fallback.onFailedAttempt

Fixes #298
---
 CHANGELOG.md                                  |  6 ++-
 .../net/jodah/failsafe/FallbackExecutor.java  | 13 +++---
 .../jodah/failsafe/issues/Issue284Test.java   |  2 +-
 .../jodah/failsafe/issues/Issue298Test.java   | 45 +++++++++++++++++++
 4 files changed, 57 insertions(+), 9 deletions(-)
 create mode 100644 src/test/java/net/jodah/failsafe/issues/Issue298Test.java

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 8b5f8a09..b4f37cd7 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,4 +1,8 @@
-# Next Minor Release
+# 2.4.4
+
+### Bug Fixes
+
+- Fixed #298 - `Fallback.onFailedAttempt` not being called correctly
 
 ### API Changes
 
diff --git a/src/main/java/net/jodah/failsafe/FallbackExecutor.java b/src/main/java/net/jodah/failsafe/FallbackExecutor.java
index 972a0f2c..09366d00 100644
--- a/src/main/java/net/jodah/failsafe/FallbackExecutor.java
+++ b/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;
-  }
 }
diff --git a/src/test/java/net/jodah/failsafe/issues/Issue284Test.java b/src/test/java/net/jodah/failsafe/issues/Issue284Test.java
index 79ca34bf..e91674ef 100644
--- a/src/test/java/net/jodah/failsafe/issues/Issue284Test.java
+++ b/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");
   }
 
diff --git a/src/test/java/net/jodah/failsafe/issues/Issue298Test.java b/src/test/java/net/jodah/failsafe/issues/Issue298Test.java
new file mode 100644
index 00000000..6f8dcb87
--- /dev/null
+++ b/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());
+  }
+}