Fix/improve retry-predicate:

- If get timeout of 0 (or negative), then still try once.
- Remove (unlikely) race in retry’s apply(T) where context-switching
  delays could cause `before(end)` to return false the first time, even
  though the timeout was positive.
- Ensure retries at end of the timeout (e.g. if timeout is 30 secs
  and last sleep takes us up to the 30 secs mark, then test again
  rather than returning immediately after the sleep!)
- Use `long` for time, rather than `java.util.Date`, for internal
  calculations. Deprecates old protected methods that use Date.

core/src/main/java/org/jclouds/util/Predicates2.java

@@ -104,19 +104,30 @@ public class Predicates2 {
       
       @Override
       public boolean apply(T input) {
+         // Ensure that try one last time at the end (e.g. if timeout is 30 secs and last sleep
+         // takes us up to the 30 secs mark, then test again rather than returning immediately 
+         // after the sleep!).
+         // Always try at least once, even if timeout is 0 or negative.
+         // A timeout of 0 means 0 millis.
          try {
             long i = 1l;
-            for (Date end = new Date(System.currentTimeMillis() + timeout); before(end); Thread.sleep(nextMaxInterval(i++,
+            long now = System.currentTimeMillis();
-                     end))) {
+            long end = now + timeout;
+            while (now < end) {
                if (findOrBreak.apply(input)) {
                   return true;
-               } else if (atOrAfter(end)) {
-                  return false;
                }
+               long sleepTime = nextMaxInterval(i++, end);
+               if (sleepTime > 0) Thread.sleep(sleepTime);
+               now = System.currentTimeMillis();
             }
+
+            return findOrBreak.apply(input);
+
          } catch (InterruptedException e) {
             logger.warn(e, "predicate %s on %s interrupted, returning false", input, findOrBreak);
             Thread.currentThread().interrupt();
+            return false;
          } catch (RuntimeException e) {
             if (getFirstThrowableOfType(e, ExecutionException.class) != null) {
                logger.warn(e, "predicate %s on %s errored [%s], returning false", input, findOrBreak, e.getMessage());
@@ -133,7 +144,13 @@ public class Predicates2 {
             } else
                throw e;
          }
-         return false;
+      }
+
+      /**
+       * @deprecated since 1.9.0; use {@link #nextMaxInterval(long, long)}
+       */
+      protected long nextMaxInterval(long attempt, Date end) {
+         return nextMaxInterval(attempt, end.getTime());
       }
 
       /**
@@ -142,21 +159,27 @@ public class Predicates2 {
        * (where 1.5 is the backoff factor), to the maximum interval or specified timeout.
        * 
        * @param attempt number of this attempt (starting at 1 for the first retry)
-       * @param end timeout
+       * @param endTime timeout
        * @return time in milliseconds from now until the next attempt, or if negative, time lapsed
        * since the specified timeout
        */
-      protected long nextMaxInterval(long attempt, Date end) {
+      protected long nextMaxInterval(long attempt, long endTime) {
          long interval = (long) (period * Math.pow(1.5, attempt - 1));
          interval = interval > maxPeriod ? maxPeriod : interval;
-         long max = end.getTime() - System.currentTimeMillis();
+         long max = endTime - System.currentTimeMillis();
          return (interval > max) ? max : interval;
       }
 
+      /**
+       * @deprecated since 1.9.0; use {@link #nextMaxInterval(long, long)}
+       */
       protected boolean before(Date end) {
          return new Date().compareTo(end) <= 1;
       }
 
+      /**
+       * @deprecated since 1.9.0; use {@link #nextMaxInterval(long, long)}
+       */
       protected boolean atOrAfter(Date end) {
          return new Date().compareTo(end) >= 0;
       }

core/src/test/java/org/jclouds/util/Predicates2Test.java

@@ -19,6 +19,8 @@ package org.jclouds.util;
 import static java.util.concurrent.TimeUnit.MILLISECONDS;
 import static java.util.concurrent.TimeUnit.SECONDS;
 import static org.jclouds.util.Predicates2.retry;
+import static org.testng.Assert.assertFalse;
+import static org.testng.Assert.assertTrue;
 import static org.testng.Assert.fail;
 
 import java.util.Arrays;
@@ -31,7 +33,6 @@ import org.testng.annotations.BeforeMethod;
 import org.testng.annotations.Test;
 
 import com.google.common.base.Predicate;
-import com.google.common.base.Predicates;
 import com.google.common.base.Stopwatch;
 import com.google.common.base.Supplier;
 import com.google.common.collect.Lists;
@@ -52,29 +53,23 @@ public class Predicates2Test {
    }
    
    @Test
-   void testFalseOnIllegalStateExeception() {
+   void testRetryReturnsFalseOnIllegalStateExeception() {
       ensureImmediateReturnFor(new IllegalStateException());
    }
 
-   @SuppressWarnings("serial")
    @Test
-   void testFalseOnExecutionException() {
+   void testRetryReturnsFalseOnExecutionException() {
-      ensureImmediateReturnFor(new ExecutionException() {
+      ensureImmediateReturnFor(new ExecutionException(new Exception("Simulated cause")));
-      });
    }
 
-   @SuppressWarnings("serial")
    @Test
-   void testFalseOnTimeoutException() {
+   void testRetryReturnsFalseOnTimeoutException() {
-      ensureImmediateReturnFor(new TimeoutException() {
+      ensureImmediateReturnFor(new TimeoutException("Simulating exception"));
-      });
    }
 
-   @SuppressWarnings("serial")
    @Test(expectedExceptions = RuntimeException.class)
-   void testPropagateOnException() {
+   void testRetryPropagatesOnException() {
-      ensureImmediateReturnFor(new Exception() {
+      ensureImmediateReturnFor(new Exception("Simulating exception"));
-      });
    }
 
    private void ensureImmediateReturnFor(final Exception ex) {
@@ -86,48 +81,66 @@ public class Predicates2Test {
                }, 3, 1, SECONDS);
       
       stopwatch.start();
-      assert !predicate.apply(new Supplier<String>() {
+      assertFalse(predicate.apply(new Supplier<String>() {
 
          @Override
          public String get() {
             throw new RuntimeException(ex);
          }
 
-      });
+      }));
       long duration = stopwatch.elapsed(MILLISECONDS);
       assertOrdered(duration, SLOW_BUILD_SERVER_GRACE);
    }
 
    @Test
-   void testAlwaysTrue() {
+   void testRetryAlwaysFalseMillis() {
-      // will call once immediately
-      Predicate<String> predicate = retry(Predicates.<String> alwaysTrue(), 3, 1, SECONDS);
-      stopwatch.start();
-      predicate.apply("");
-      long duration = stopwatch.elapsed(MILLISECONDS);
-      assertOrdered(duration, SLOW_BUILD_SERVER_GRACE);
-   }
-
-   @Test
-   void testAlwaysFalseMillis() {
       // maxWait=3; period=1; maxPeriod defaults to 1*10
       // will call at 0, 1, 1+(1*1.5), 3
-      Predicate<String> predicate = retry(Predicates.<String> alwaysFalse(), 3, 1, SECONDS);
+      RepeatedAttemptsPredicate rawPredicate = new RepeatedAttemptsPredicate(Integer.MAX_VALUE);
+      Predicate<String> predicate = retry(rawPredicate, 3, 1, SECONDS);
       stopwatch.start();
-      predicate.apply("");
+      assertFalse(predicate.apply(""));
       long duration = stopwatch.elapsed(MILLISECONDS);
       assertOrdered(3000 - EARLY_RETURN_GRACE, duration, 3000 + SLOW_BUILD_SERVER_GRACE);
+      assertCallTimes(rawPredicate.callTimes, 0, 1000, 1000 + 1500, 3000);
    }
 
    @Test
-   void testThirdTimeTrue() {
+   void testRetryFirstTimeTrue() {
-      // maxWait=4; period=1; maxPeriod defaults to 1*10
+      RepeatedAttemptsPredicate rawPredicate = new RepeatedAttemptsPredicate(1);
-      // will call at 0, 1, 1+(1*1.5)
-      RepeatedAttemptsPredicate rawPredicate = new RepeatedAttemptsPredicate(2);
       Predicate<String> predicate = retry(rawPredicate, 4, 1, SECONDS);
 
       stopwatch.start();
-      predicate.apply("");
+      assertTrue(predicate.apply(""));
+      long duration = stopwatch.elapsed(MILLISECONDS);
+      
+      assertOrdered(0, duration, 0 + SLOW_BUILD_SERVER_GRACE);
+      assertCallTimes(rawPredicate.callTimes, 0);
+   }
+
+   @Test
+   void testRetryWillRunOnceOnNegativeTimeout() {
+      RepeatedAttemptsPredicate rawPredicate = new RepeatedAttemptsPredicate(1);
+      Predicate<String> predicate = retry(rawPredicate, -1, 1, SECONDS);
+
+      stopwatch.start();
+      assertTrue(predicate.apply(""));
+      long duration = stopwatch.elapsed(MILLISECONDS);
+      
+      assertOrdered(0, duration, 0 + SLOW_BUILD_SERVER_GRACE);
+      assertCallTimes(rawPredicate.callTimes, 0);
+   }
+
+   @Test
+   void testRetryThirdTimeTrue() {
+      // maxWait=4; period=1; maxPeriod defaults to 1*10
+      // will call at 0, 1, 1+(1*1.5)
+      RepeatedAttemptsPredicate rawPredicate = new RepeatedAttemptsPredicate(3);
+      Predicate<String> predicate = retry(rawPredicate, 4, 1, SECONDS);
+
+      stopwatch.start();
+      assertTrue(predicate.apply(""));
       long duration = stopwatch.elapsed(MILLISECONDS);
       
       assertOrdered(2500 - EARLY_RETURN_GRACE, duration, 2500 + SLOW_BUILD_SERVER_GRACE);
@@ -135,14 +148,14 @@ public class Predicates2Test {
    }
 
    @Test
-   void testThirdTimeTrueLimitedMaxInterval() {
+   void testRetryThirdTimeTrueLimitedMaxInterval() {
       // maxWait=3; period=1; maxPeriod=1
       // will call at 0, 1, 1+1
-      RepeatedAttemptsPredicate rawPredicate = new RepeatedAttemptsPredicate(2);
+      RepeatedAttemptsPredicate rawPredicate = new RepeatedAttemptsPredicate(3);
       Predicate<String> predicate = retry(rawPredicate, 3, 1, 1, SECONDS);
 
       stopwatch.start();
-      predicate.apply("");
+      assertTrue(predicate.apply(""));
       long duration = stopwatch.elapsed(MILLISECONDS);
       
       assertOrdered(2000 - EARLY_RETURN_GRACE, duration, 2000 + SLOW_BUILD_SERVER_GRACE);
@@ -163,13 +176,13 @@ public class Predicates2Test {
       @Override
       public boolean apply(String input) {
          callTimes.add(stopwatch.elapsed(MILLISECONDS));
-         return count++ == succeedOnAttempt;
+         return ++count == succeedOnAttempt;
       }
    }
    
    @Test(enabled = false) // not a test, but picked up as such because public
    public static void assertCallTimes(List<Long> actual, Integer... expected) {
-      Assert.assertEquals(actual.size(), expected.length);
+      Assert.assertEquals(actual.size(), expected.length, "actual=" + actual);
       for (int i = 0; i < expected.length; i++) {
          long callTime = actual.get(i);
          assertOrdered(expected[i] - EARLY_RETURN_GRACE, callTime, expected[i] + SLOW_BUILD_SERVER_GRACE);