From 424f2b35d3652d2b3c1247b1084fe048f3c0fe2d Mon Sep 17 00:00:00 2001
From: Jonathan Halterman <jhalterman@gmail.com>
Date: Fri, 22 Jul 2016 12:23:40 -0700
Subject: [PATCH] Failed attempt listener not always called on completion. Re:
 issue #36.

---
 .../net/jodah/failsafe/AbstractExecution.java | 11 +++--
 .../java/net/jodah/failsafe/RetryPolicy.java  | 28 +++++------
 .../net/jodah/failsafe/issues/Issue36.java    | 49 +++++++++++++++++++
 3 files changed, 68 insertions(+), 20 deletions(-)
 create mode 100644 src/test/java/net/jodah/failsafe/issues/Issue36.java

diff --git a/src/main/java/net/jodah/failsafe/AbstractExecution.java b/src/main/java/net/jodah/failsafe/AbstractExecution.java
index a36d9656..9f874e3a 100644
--- a/src/main/java/net/jodah/failsafe/AbstractExecution.java
+++ b/src/main/java/net/jodah/failsafe/AbstractExecution.java
@@ -106,16 +106,17 @@ boolean complete(Object result, Throwable failure, boolean checkArgs) {
     boolean maxDurationExceeded = retryPolicy.getMaxDuration() != null
         && elapsedNanos > retryPolicy.getMaxDuration().toNanos();
     retriesExceeded = maxRetriesExceeded || maxDurationExceeded;
-    boolean shouldAbort = retryPolicy.canAbortFor(result, failure);
-    boolean shouldRetry = !retriesExceeded && !shouldAbort && checkArgs && retryPolicy.canRetryFor(result, failure);
-    completed = shouldAbort || !shouldRetry;
-    success = completed && !shouldRetry && !shouldAbort && failure == null;
+    boolean isAbortable = retryPolicy.canAbortFor(result, failure);
+    boolean isRetryable = retryPolicy.canRetryFor(result, failure);
+    boolean shouldRetry = !retriesExceeded && checkArgs && !isAbortable && retryPolicy.allowsRetries() && isRetryable;
+    completed = isAbortable || !shouldRetry;
+    success = completed && !isAbortable && !isRetryable && failure == null;
 
     // Call listeners
     if (listeners != null) {
       if (!success)
         listeners.handleFailedAttempt(result, failure, this);
-      if (shouldAbort)
+      if (isAbortable)
         listeners.handleAbort(result, failure, this);
       else {
         if (retriesExceeded)
diff --git a/src/main/java/net/jodah/failsafe/RetryPolicy.java b/src/main/java/net/jodah/failsafe/RetryPolicy.java
index 3e8fd3c5..0452641c 100644
--- a/src/main/java/net/jodah/failsafe/RetryPolicy.java
+++ b/src/main/java/net/jodah/failsafe/RetryPolicy.java
@@ -129,8 +129,18 @@ public RetryPolicy abortWhen(Object result) {
   }
 
   /**
-   * Returns whether an execution can be aborted for the {@code result} and {@code failure} according to the configured
-   * abort conditions.
+   * Returns whether the policy allows retries according to the configured {@link #withMaxRetries(int) maxRetries} and
+   * {@link #withMaxDuration(long, TimeUnit) maxDuration}.
+   * 
+   * @see #withMaxRetries(int)
+   * @see #withMaxDuration(long, TimeUnit)
+   */
+  public boolean allowsRetries() {
+    return (maxRetries == -1 || maxRetries > 0) && (maxDuration == null || maxDuration.toNanos() > 0);
+  }
+
+  /**
+   * Returns whether an execution result can be aborted given the configured abort conditions.
    * 
    * @see #abortIf(BiPredicate)
    * @see #abortIf(Predicate)
@@ -148,16 +158,7 @@ public boolean canAbortFor(Object result, Throwable failure) {
   }
 
   /**
-   * Returns whether an execution can be retried according to the configured maxRetries and maxDuration.
-   */
-  public boolean canRetry() {
-    return (maxRetries == -1 || maxRetries > 0) && (maxDuration == null || maxDuration.toNanos() > 0);
-  }
-
-  /**
-   * Returns whether an execution can be retried for the {@code result} and {@code failure} according to configured
-   * retry conditions, or if the {@code failure} is not null and no retry condition has been configured to check
-   * failures.
+   * Returns whether an execution result can be retried given the configured abort conditions.
    * 
    * @see #retryIf(BiPredicate)
    * @see #retryIf(Predicate)
@@ -167,9 +168,6 @@ public boolean canRetry() {
    * @see #retryWhen(Object)
    */
   public boolean canRetryFor(Object result, Throwable failure) {
-    if (!canRetry())
-      return false;
-
     for (BiPredicate<Object, Throwable> predicate : retryConditions) {
       if (predicate.test(result, failure))
         return true;
diff --git a/src/test/java/net/jodah/failsafe/issues/Issue36.java b/src/test/java/net/jodah/failsafe/issues/Issue36.java
new file mode 100644
index 00000000..e32584c5
--- /dev/null
+++ b/src/test/java/net/jodah/failsafe/issues/Issue36.java
@@ -0,0 +1,49 @@
+package net.jodah.failsafe.issues;
+
+import static org.testng.Assert.assertEquals;
+
+import java.util.concurrent.atomic.AtomicInteger;
+
+import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.Test;
+
+import net.jodah.failsafe.Failsafe;
+import net.jodah.failsafe.RetryPolicy;
+
+/**
+ * https://github.com/jhalterman/failsafe/issues/36
+ */
+@Test
+@SuppressWarnings("unchecked")
+public class Issue36 {
+  RetryPolicy retryPolicy = new RetryPolicy().retryWhen(false).retryOn(Exception.class).withMaxRetries(3);
+  AtomicInteger calls;
+  AtomicInteger failedAttempts;
+  AtomicInteger retries;
+
+  @BeforeMethod
+  protected void beforeMethod() {
+    calls = new AtomicInteger();
+    failedAttempts = new AtomicInteger();
+    retries = new AtomicInteger();
+  }
+
+  public void test() {
+    Failsafe.with(retryPolicy)
+        .onFailedAttempt(e -> failedAttempts.incrementAndGet())
+        .onRetry(e -> retries.incrementAndGet())
+        .get(() -> {
+          calls.incrementAndGet();
+          return false;
+        });
+
+    // Then
+    assertCounters();
+  }
+
+  void assertCounters() {
+    assertEquals(calls.get(), 4);
+    assertEquals(failedAttempts.get(), 4);
+    assertEquals(retries.get(), 3);
+  }
+}