Merge pull request #151 from rhusar/150-unlimited-retries

Fixes #150: @Retry(maxRetries = -1) never retries
smallrye · Dec 9, 2019 · 98a0469 · 98a0469
2 parents 099e3cb + 0113cf2
commit 98a0469
Show file tree

Hide file tree

Showing 3 changed files with 36 additions and 10 deletions.
diff --git a/implementation/src/main/java/io/smallrye/faulttolerance/RetryContext.java b/implementation/src/main/java/io/smallrye/faulttolerance/RetryContext.java
@@ -39,6 +39,8 @@ public class RetryContext {
 
     private final long delay;
 
+    private boolean hasBeenRetried;
+
     RetryContext(RetryConfig config) {
         this.config = config;
         this.start = System.nanoTime();
@@ -61,7 +63,10 @@ Exception nextRetry(Throwable throwable) {
         // Check the exception type
         if (shouldRetryOn(throwable)) {
             shouldRetry.set(null);
-            remainingAttempts.decrementAndGet();
+            if (remainingAttempts.get() != -1) {
+                remainingAttempts.decrementAndGet();
+            }
+            hasBeenRetried = true;
             return delayIfNeeded();
         } else {
             if (throwable instanceof Error) {
@@ -76,7 +81,7 @@ Exception nextRetry(Throwable throwable) {
     }
 
     boolean shouldRetry() {
-        return remainingAttempts.get() > 0;
+        return (remainingAttempts.get() == -1) || remainingAttempts.get() > 0;
     }
 
     public boolean isLastAttempt() {
@@ -130,11 +135,11 @@ private Exception delayIfNeeded() {
 
     @Override
     public String toString() {
-        return "RetryContext [remainingAttempts=" + remainingAttempts + ", start=" + start + "]";
+        return "RetryContext [remainingAttempts=" + remainingAttempts + ", hasBeenRetried=" + hasBeenRetried + ", start=" + start + "]";
     }
 
     public boolean hasBeenRetried() {
-        return remainingAttempts.get() < (config.<Integer> get(RetryConfig.MAX_RETRIES));
+        return hasBeenRetried;
     }
 
     public void cancel() {

diff --git a/testsuite/basic/src/test/java/io/smallrye/faulttolerance/retry/RetryTest.java b/testsuite/basic/src/test/java/io/smallrye/faulttolerance/retry/RetryTest.java
@@ -33,13 +33,15 @@
 import org.jboss.arquillian.junit.Arquillian;
 import org.jboss.shrinkwrap.api.spec.JavaArchive;
 import org.junit.After;
+import org.junit.Assert;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 
 import io.smallrye.faulttolerance.TestArchive;
 
 /**
  * @author Michal Szynkiewicz, michal.l.szynkiewicz@gmail.com
+ * @author Radoslav Husar
  */
 @RunWith(Arquillian.class)
 public class RetryTest {
@@ -58,6 +60,16 @@ public void cleanUp() {
         retryTestBean.reset();
     }
 
+    @Test
+    public void shouldRetryIndefinitely() {
+        try {
+            retryTestBean.callWithUnlimitedRetries();
+        } catch (IllegalArgumentException ignore) {
+            return;
+        }
+        Assert.fail();
+    }
+
     @Test
     public void shouldFallbackOnMaxDurationExceeded() {
         String result = retryTestBean.callWithMaxDuration500ms(600L);
@@ -87,7 +99,7 @@ public void shouldRetryOnBulkheadExceptionIfSpecified() throws InterruptedExcept
         ExecutorService executorService = Executors.newFixedThreadPool(3);
         Callable<String> call = () -> retryTestBean.callWithRetryOnBulkhead();
         List<Future<String>> futures = executorService.invokeAll(asList(call, call, call));
-        List<String> results = collectResultsAsummingFailures(futures, 0);
+        List<String> results = collectResultsAssumingFailures(futures, 0);
 
         assertEquals("There were " + results.size() + " results instead of 3", 3, results.size());
         assertTrue("first call failed and was expected to be successful", results.contains("call0"));
@@ -101,7 +113,7 @@ public void shouldNotRetryOnBulkheadExceptionIfNotSpecified() throws Interrupted
         Callable<String> call = () -> retryTestBean.callWithNoRetryOnBulkhead();
         List<Future<String>> futures = executorService.invokeAll(asList(call, call, call));
 
-        List<String> results = collectResultsAsummingFailures(futures, 1);
+        List<String> results = collectResultsAssumingFailures(futures, 1);
         assertEquals(2, results.size());
         assertTrue("first call failed and was expected to be successful", results.contains("call0"));
         assertTrue("second call failed and was expected to be successful", results.contains("call1"));
@@ -113,15 +125,15 @@ public void shouldFallbackOnNoRetryOnBulkhead() throws InterruptedException {
         Callable<String> call = () -> retryTestBean.callWithFallbackAndNoRetryOnBulkhead();
         List<Future<String>> futures = executorService.invokeAll(asList(call, call, call));
 
-        List<String> results = collectResultsAsummingFailures(futures, 0);
+        List<String> results = collectResultsAssumingFailures(futures, 0);
         assertEquals(3, results.size());
         assertTrue("first call failed and was expected to be successful", results.contains("call0"));
         assertTrue("second call failed and was expected to be successful", results.contains("call1"));
         assertTrue("third call didn't fall back",
                 results.stream().anyMatch(t -> t.startsWith("fallback")));
     }
 
-    private List<String> collectResultsAsummingFailures(List<Future<String>> futures, int expectedFailureCount) {
+    private List<String> collectResultsAssumingFailures(List<Future<String>> futures, int expectedFailureCount) {
         int failureCount = 0;
         List<String> resultList = new ArrayList<>();
         for (Future<String> future : futures) {

diff --git a/testsuite/basic/src/test/java/io/smallrye/faulttolerance/retry/RetryTestBean.java b/testsuite/basic/src/test/java/io/smallrye/faulttolerance/retry/RetryTestBean.java
@@ -28,14 +28,23 @@
 
 /**
  * @author Michal Szynkiewicz, michal.l.szynkiewicz@gmail.com
+ * @author Radoslav Husar
  */
 @Dependent
 public class RetryTestBean {
 
-    private Object bulkheadControl = new Object();
-
     private AtomicInteger attempt = new AtomicInteger();
 
+    @Retry(maxRetries = -1, retryOn = NullPointerException.class, abortOn = IllegalArgumentException.class)
+    public void callWithUnlimitedRetries() {
+        int attempt = this.attempt.getAndIncrement();
+        if (attempt < 5) {
+            throw new NullPointerException();
+        } else {
+            throw new IllegalArgumentException();
+        }
+    }
+
     @Retry(maxDuration = 500L, maxRetries = 2)
     @Fallback(fallbackMethod = "fallback")
     @Timeout(500L)