Merge pull request #66 from pwhittlesea/master

Fix #65: Ensure beforeNextTry is only called before a retry

src/main/java/com/evanlennick/retry4j/CallExecutor.java

@@ -72,14 +72,17 @@ public Status<T> execute(Callable<T> callable, String callName) {
 
         try {
             for (tries = 0; tries < maxTries && !attemptStatus.wasSuccessful(); tries++) {
+                if (tries > 0) {
+                    handleBeforeNextTry(millisBetweenTries, tries);
+                    logger.trace("Retry4j retrying for time number {}", tries);
+                }
+
                 logger.trace("Retry4j executing callable {}", callable);
                 attemptStatus = tryCall(callable);
 
                 if (!attemptStatus.wasSuccessful()) {
-                    handleRetry(millisBetweenTries, tries + 1);
+                    handleFailedTry(tries + 1);
                 }
-
-                logger.trace("Retry4j retrying for time number {}", tries);
             }
 
             refreshRetryStatus(attemptStatus.wasSuccessful(), tries);
@@ -142,18 +145,19 @@ private AttemptStatus<T> tryCall(Callable<T> callable) throws UnexpectedExceptio
         return attemptStatus;
     }
 
-    private void handleRetry(long millisBetweenTries, int tries) {
+    private void handleBeforeNextTry(final long millisBetweenTries, final int tries) {
+        sleep(millisBetweenTries, tries);
+        if (null != beforeNextTryListener) {
+            beforeNextTryListener.onEvent(status);
+        }
+    }
+
+    private void handleFailedTry(int tries) {
         refreshRetryStatus(false, tries);
 
         if (null != afterFailedTryListener) {
             afterFailedTryListener.onEvent(status);
         }
-
-        sleep(millisBetweenTries, tries);
-
-        if (null != beforeNextTryListener) {
-            beforeNextTryListener.onEvent(status);
-        }
     }
 
     private void refreshRetryStatus(boolean success, int tries) {

src/test/java/com/evanlennick/retry4j/CallExecutorTest.java

@@ -1,28 +1,40 @@
 package com.evanlennick.retry4j;
 
+import com.evanlennick.retry4j.backoff.BackoffStrategy;
 import com.evanlennick.retry4j.config.RetryConfig;
 import com.evanlennick.retry4j.config.RetryConfigBuilder;
 import com.evanlennick.retry4j.exception.RetriesExhaustedException;
 import com.evanlennick.retry4j.exception.UnexpectedException;
+
+import org.mockito.Mock;
+import org.mockito.MockitoAnnotations;
 import org.testng.annotations.BeforeMethod;
 import org.testng.annotations.Test;
 
 import java.io.FileNotFoundException;
 import java.io.IOException;
+import java.time.Duration;
 import java.time.temporal.ChronoUnit;
 import java.util.Random;
 import java.util.concurrent.Callable;
 
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.fail;
 import static org.assertj.core.api.Assertions.within;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
 
 public class CallExecutorTest {
 
+    @Mock
+    private BackoffStrategy mockBackOffStrategy;
+
     private RetryConfigBuilder retryConfigBuilder;
 
     @BeforeMethod
     public void setup() {
+        MockitoAnnotations.initMocks(this);
+
         boolean configValidationEnabled = false;
         this.retryConfigBuilder = new RetryConfigBuilder(configValidationEnabled);
     }
@@ -221,4 +233,29 @@ public void verifyRetryingIndefinitely() throws Exception {
             fail("Retries should never be exhausted!");
         }
     }
+
+    @Test
+    public void verifyRetryPolicyTimeoutIsUsed() {
+        Callable<Object> callable = () -> {
+            throw new RuntimeException();
+        };
+
+        Duration delayBetweenTriesDuration = Duration.ofSeconds(17);
+        when(mockBackOffStrategy.getDurationToWait(1, delayBetweenTriesDuration)).thenReturn(Duration.ofSeconds(5));
+
+        RetryConfig retryConfig = retryConfigBuilder
+                .withMaxNumberOfTries(2)
+                .retryOnAnyException()
+                .withDelayBetweenTries(delayBetweenTriesDuration)
+                .withBackoffStrategy(mockBackOffStrategy)
+                .build();
+
+        final long before = System.currentTimeMillis();
+        try {
+            new CallExecutor<>(retryConfig).execute(callable);
+        } catch (RetriesExhaustedException ignored) {}
+
+        assertThat(System.currentTimeMillis() - before).isGreaterThan(5000);
+        verify(mockBackOffStrategy).getDurationToWait(1, delayBetweenTriesDuration);
+    }
 }

src/test/java/com/evanlennick/retry4j/CallExecutorTest_ListenersTest.java

@@ -10,6 +10,7 @@
 import java.time.temporal.ChronoUnit;
 import java.util.concurrent.Callable;
 
+import static org.mockito.Mockito.after;
 import static org.mockito.Mockito.isA;
 import static org.mockito.Mockito.timeout;
 import static org.mockito.Mockito.verify;
@@ -78,6 +79,25 @@ public void verifyBeforeNextTryListener() {
         verify(dummyMock, timeout(1000).times(2)).listenersCallThis();
     }
 
+    @Test
+    public void verifyBeforeNextTryListener_NotCalledAfterLastFailure() {
+        when(dummyMock.callableCallThis())
+                .thenThrow(new NullPointerException())
+                .thenThrow(new NullPointerException())
+                .thenThrow(new NullPointerException())
+                .thenThrow(new NullPointerException())
+                .thenThrow(new IllegalArgumentException());
+
+        executor.beforeNextTry(status -> dummyMock.listenersCallThis(status.getLastExceptionThatCausedRetry()));
+        try {
+            executor.execute(callable);
+        } catch (Exception e) {}
+
+        // the beforeNextTry listener should only be called before a retry (5 attempts == 4 retries)
+        verify(dummyMock, timeout(1000).times(4)).listenersCallThis(isA(NullPointerException.class));
+        verify(dummyMock, after(1000).never()).listenersCallThis(isA(IllegalArgumentException.class));
+    }
+
     @Test
     public void verifyOnSuccessListener() {
         when(dummyMock.callableCallThis())