From 7866612a2eca6e3ee32bc7928e692ec325ac7d30 Mon Sep 17 00:00:00 2001 From: Aled Sage Date: Tue, 27 Jan 2015 11:44:37 +0000 Subject: [PATCH] Fix/improve retry-predicate: MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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. --- .../java/org/jclouds/util/Predicates2.java | 39 ++++++-- .../org/jclouds/util/Predicates2Test.java | 91 +++++++++++-------- 2 files changed, 83 insertions(+), 47 deletions(-) diff --git a/core/src/main/java/org/jclouds/util/Predicates2.java b/core/src/main/java/org/jclouds/util/Predicates2.java index 738e3a9eaa..3c0b30304c 100644 --- a/core/src/main/java/org/jclouds/util/Predicates2.java +++ b/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++, - end))) { + long now = System.currentTimeMillis(); + 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; } diff --git a/core/src/test/java/org/jclouds/util/Predicates2Test.java b/core/src/test/java/org/jclouds/util/Predicates2Test.java index 0b6a936503..66e610de73 100644 --- a/core/src/test/java/org/jclouds/util/Predicates2Test.java +++ b/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() { - ensureImmediateReturnFor(new ExecutionException() { - }); + void testRetryReturnsFalseOnExecutionException() { + ensureImmediateReturnFor(new ExecutionException(new Exception("Simulated cause"))); } - @SuppressWarnings("serial") @Test - void testFalseOnTimeoutException() { - ensureImmediateReturnFor(new TimeoutException() { - }); + void testRetryReturnsFalseOnTimeoutException() { + ensureImmediateReturnFor(new TimeoutException("Simulating exception")); } - @SuppressWarnings("serial") @Test(expectedExceptions = RuntimeException.class) - void testPropagateOnException() { - ensureImmediateReturnFor(new Exception() { - }); + void testRetryPropagatesOnException() { + 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() { + assertFalse(predicate.apply(new Supplier() { @Override public String get() { throw new RuntimeException(ex); } - }); + })); long duration = stopwatch.elapsed(MILLISECONDS); assertOrdered(duration, SLOW_BUILD_SERVER_GRACE); } @Test - void testAlwaysTrue() { - // will call once immediately - Predicate predicate = retry(Predicates. alwaysTrue(), 3, 1, SECONDS); - stopwatch.start(); - predicate.apply(""); - long duration = stopwatch.elapsed(MILLISECONDS); - assertOrdered(duration, SLOW_BUILD_SERVER_GRACE); - } - - @Test - void testAlwaysFalseMillis() { + void testRetryAlwaysFalseMillis() { // maxWait=3; period=1; maxPeriod defaults to 1*10 // will call at 0, 1, 1+(1*1.5), 3 - Predicate predicate = retry(Predicates. alwaysFalse(), 3, 1, SECONDS); + RepeatedAttemptsPredicate rawPredicate = new RepeatedAttemptsPredicate(Integer.MAX_VALUE); + Predicate 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() { - // maxWait=4; period=1; maxPeriod defaults to 1*10 - // will call at 0, 1, 1+(1*1.5) - RepeatedAttemptsPredicate rawPredicate = new RepeatedAttemptsPredicate(2); + void testRetryFirstTimeTrue() { + RepeatedAttemptsPredicate rawPredicate = new RepeatedAttemptsPredicate(1); Predicate 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 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 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 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 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);