fix: allows retries when setting maxAttempts without setting TotalTimeout

gax/src/main/java/com/google/api/gax/retrying/ExponentialRetryAlgorithm.java

@@ -130,14 +130,32 @@ public TimedAttemptSettings createNextAttempt(TimedAttemptSettings prevSettings)
   @Override
   public boolean shouldRetry(TimedAttemptSettings nextAttemptSettings) {
     RetrySettings globalSettings = nextAttemptSettings.getGlobalSettings();
+
+    int maxAttempts = globalSettings.getMaxAttempts();
+    long totalTimeout = globalSettings.getTotalTimeout().toNanos();
+
+    // If total timeout and maxAttempts is not set then do not attempt retry.
+    if (totalTimeout == 0 && maxAttempts == 0) {
+      return false;
+    }
+
     long totalTimeSpentNanos =
         clock.nanoTime()
             - nextAttemptSettings.getFirstAttemptStartTimeNanos()
             + nextAttemptSettings.getRandomizedRetryDelay().toNanos();
 
-    return totalTimeSpentNanos <= globalSettings.getTotalTimeout().toNanos()
-        && (globalSettings.getMaxAttempts() <= 0
-            || nextAttemptSettings.getAttemptCount() < globalSettings.getMaxAttempts());
+    // If totalTimeout limit is defined, check that it hasn't been crossed
+    if (totalTimeout > 0 && totalTimeSpentNanos > totalTimeout) {
+      return false;
+    }
+
+    // If maxAttempts limit is defined, check that it hasn't been crossed
+    if (maxAttempts > 0 && nextAttemptSettings.getAttemptCount() >= maxAttempts) {
+      return false;
+    }
+
+    // No limits crossed
+    return true;
   }
 
   // Injecting Random is not possible here, as Random does not provide nextLong(long bound) method

gax/src/main/java/com/google/api/gax/retrying/RetrySettings.java

@@ -41,9 +41,11 @@
  * fail (and return an error code) or not respond (and cause a timeout). When there is a failure or
  * timeout, the logic should keep trying until the total timeout has passed.
  *
- * <p>The "total timeout" parameter has ultimate control over how long the logic should keep trying
- * the remote call until it gives up completely. The higher the total timeout, the more retries can
- * be attempted. The other settings are considered more advanced.
+ * <p>The "total timeout" and "max attempts" settings have ultimate control over how long the logic
+ * should keep trying the remote call until it gives up completely. The remote call will be retried
+ * until one of those thresholds is crossed. To avoid unbounded rpc calls, it is required to
+ * configure one of those settings. If both are 0, then retries will be disabled. The other settings
+ * are considered more advanced.
  *
  * <p>Retry delay and timeout start at specific values, and are tracked separately from each other.
  * The very first call (before any retries) will use the initial timeout.

gax/src/main/java/com/google/api/gax/rpc/Callables.java

@@ -248,6 +248,8 @@ OperationCallableImpl<RequestT, ResponseT, MetadataT> longRunningOperationImpl(
 
   private static boolean areRetriesDisabled(
       Collection<StatusCode.Code> retryableCodes, RetrySettings retrySettings) {
-    return retrySettings.getMaxAttempts() == 1 || retryableCodes.isEmpty();
+    return retrySettings.getMaxAttempts() == 1
+        || retryableCodes.isEmpty()
+        || (retrySettings.getMaxAttempts() == 0 && retrySettings.getTotalTimeout().isZero());
   }
 }

gax/src/test/java/com/google/api/gax/rpc/RetryingTest.java

@@ -109,10 +109,7 @@ public void retry() {
         .thenReturn(RetryingTest.<Integer>immediateFailedFuture(throwable))
         .thenReturn(ApiFutures.<Integer>immediateFuture(2));
 
-    UnaryCallSettings<Integer, Integer> callSettings = createSettings(FAST_RETRY_SETTINGS);
-    UnaryCallable<Integer, Integer> callable =
-        FakeCallableFactory.createUnaryCallable(callInt, callSettings, clientContext);
-    Truth.assertThat(callable.call(1)).isEqualTo(2);
+    assertRetrying(FAST_RETRY_SETTINGS);
   }
 
   @Test(expected = ApiException.class)
@@ -129,10 +126,8 @@ public void retryTotalTimeoutExceeded() {
             .setInitialRetryDelay(Duration.ofMillis(Integer.MAX_VALUE))
             .setMaxRetryDelay(Duration.ofMillis(Integer.MAX_VALUE))
             .build();
-    UnaryCallSettings<Integer, Integer> callSettings = createSettings(retrySettings);
-    UnaryCallable<Integer, Integer> callable =
-        FakeCallableFactory.createUnaryCallable(callInt, callSettings, clientContext);
-    callable.call(1);
+
+    assertRetrying(retrySettings);
   }
 
   @Test(expected = ApiException.class)
@@ -144,11 +139,7 @@ public void retryMaxAttemptsExceeded() {
         .thenReturn(RetryingTest.<Integer>immediateFailedFuture(throwable))
         .thenReturn(ApiFutures.<Integer>immediateFuture(2));
 
-    RetrySettings retrySettings = FAST_RETRY_SETTINGS.toBuilder().setMaxAttempts(2).build();
-    UnaryCallSettings<Integer, Integer> callSettings = createSettings(retrySettings);
-    UnaryCallable<Integer, Integer> callable =
-        FakeCallableFactory.createUnaryCallable(callInt, callSettings, clientContext);
-    callable.call(1);
+    assertRetrying(FAST_RETRY_SETTINGS.toBuilder().setMaxAttempts(2).build());
   }
 
   @Test
@@ -160,12 +151,34 @@ public void retryWithinMaxAttempts() {
         .thenReturn(RetryingTest.<Integer>immediateFailedFuture(throwable))
         .thenReturn(ApiFutures.<Integer>immediateFuture(2));
 
-    RetrySettings retrySettings = FAST_RETRY_SETTINGS.toBuilder().setMaxAttempts(3).build();
-    UnaryCallSettings<Integer, Integer> callSettings = createSettings(retrySettings);
-    UnaryCallable<Integer, Integer> callable =
-        FakeCallableFactory.createUnaryCallable(callInt, callSettings, clientContext);
-    callable.call(1);
-    Truth.assertThat(callable.call(1)).isEqualTo(2);
+    assertRetrying(FAST_RETRY_SETTINGS.toBuilder().setMaxAttempts(3).build());
+  }
+
+  @Test
+  public void retryWithOnlyMaxAttempts() {
+    Throwable throwable =
+        new UnavailableException(null, FakeStatusCode.of(StatusCode.Code.UNAVAILABLE), true);
+    Mockito.when(callInt.futureCall(Mockito.<Integer>any(), Mockito.<ApiCallContext>any()))
+        .thenReturn(RetryingTest.<Integer>immediateFailedFuture(throwable))
+        .thenReturn(RetryingTest.<Integer>immediateFailedFuture(throwable))
+        .thenReturn(ApiFutures.immediateFuture(2));
+
+    RetrySettings retrySettings = RetrySettings.newBuilder().setMaxAttempts(3).build();
+
+    assertRetrying(retrySettings);
+    Mockito.verify(callInt, Mockito.times(3))
+        .futureCall(Mockito.<Integer>any(), Mockito.<ApiCallContext>any());
+  }
+
+  @Test
+  public void retryWithoutRetrySettings() {
+    Mockito.when(callInt.futureCall(Mockito.<Integer>any(), Mockito.<ApiCallContext>any()))
+        .thenReturn(ApiFutures.immediateFuture(2));
+
+    RetrySettings retrySettings = RetrySettings.newBuilder().build();
+
+    assertRetrying(retrySettings);
+    Mockito.verify(callInt).futureCall(Mockito.<Integer>any(), Mockito.<ApiCallContext>any());
   }
 
   @Test
@@ -177,10 +190,8 @@ public void retryOnStatusUnknown() {
         .thenReturn(RetryingTest.<Integer>immediateFailedFuture(throwable))
         .thenReturn(RetryingTest.<Integer>immediateFailedFuture(throwable))
         .thenReturn(ApiFutures.<Integer>immediateFuture(2));
-    UnaryCallSettings<Integer, Integer> callSettings = createSettings(FAST_RETRY_SETTINGS);
-    UnaryCallable<Integer, Integer> callable =
-        FakeCallableFactory.createUnaryCallable(callInt, callSettings, clientContext);
-    Truth.assertThat(callable.call(1)).isEqualTo(2);
+
+    assertRetrying(FAST_RETRY_SETTINGS);
   }
 
   @Test
@@ -191,10 +202,7 @@ public void retryOnUnexpectedException() {
         new UnknownException("foobar", null, FakeStatusCode.of(StatusCode.Code.UNKNOWN), false);
     Mockito.when(callInt.futureCall((Integer) Mockito.any(), (ApiCallContext) Mockito.any()))
         .thenReturn(RetryingTest.<Integer>immediateFailedFuture(throwable));
-    UnaryCallSettings<Integer, Integer> callSettings = createSettings(FAST_RETRY_SETTINGS);
-    UnaryCallable<Integer, Integer> callable =
-        FakeCallableFactory.createUnaryCallable(callInt, callSettings, clientContext);
-    callable.call(1);
+    assertRetrying(FAST_RETRY_SETTINGS);
   }
 
   @Test
@@ -207,10 +215,7 @@ public void retryNoRecover() {
                 new FailedPreconditionException(
                     "foobar", null, FakeStatusCode.of(StatusCode.Code.FAILED_PRECONDITION), false)))
         .thenReturn(ApiFutures.<Integer>immediateFuture(2));
-    UnaryCallSettings<Integer, Integer> callSettings = createSettings(FAST_RETRY_SETTINGS);
-    UnaryCallable<Integer, Integer> callable =
-        FakeCallableFactory.createUnaryCallable(callInt, callSettings, clientContext);
-    callable.call(1);
+    assertRetrying(FAST_RETRY_SETTINGS);
   }
 
   @Test
@@ -272,4 +277,11 @@ public static UnaryCallSettings<Integer, Integer> createSettings(RetrySettings r
         .setRetrySettings(retrySettings)
         .build();
   }
+
+  private void assertRetrying(RetrySettings retrySettings) {
+    UnaryCallSettings<Integer, Integer> callSettings = createSettings(retrySettings);
+    UnaryCallable<Integer, Integer> callable =
+        FakeCallableFactory.createUnaryCallable(callInt, callSettings, clientContext);
+    Truth.assertThat(callable.call(1)).isEqualTo(2);
+  }
 }