From 03e1eaf6508c1c4a585e103fbcf22954d1b6d6d3 Mon Sep 17 00:00:00 2001 From: Martin Derka Date: Fri, 8 Apr 2016 14:04:35 -0700 Subject: [PATCH] RetryHelper can be configured to not retry on socket timeouts. This fixes #816 and #808. --- .../java/com/google/gcloud/RetryHelper.java | 24 ++++++++++----- .../com/google/gcloud/RetryHelperTest.java | 29 ++++++++++++++++++- .../java/com/google/gcloud/dns/DnsImpl.java | 6 ++-- 3 files changed, 48 insertions(+), 11 deletions(-) diff --git a/gcloud-java-core/src/main/java/com/google/gcloud/RetryHelper.java b/gcloud-java-core/src/main/java/com/google/gcloud/RetryHelper.java index 9b9c1f6a3124..4b2cbb0455f8 100644 --- a/gcloud-java-core/src/main/java/com/google/gcloud/RetryHelper.java +++ b/gcloud-java-core/src/main/java/com/google/gcloud/RetryHelper.java @@ -29,6 +29,7 @@ import com.google.common.base.Stopwatch; import java.io.InterruptedIOException; +import java.net.SocketTimeoutException; import java.nio.channels.ClosedByInterruptException; import java.util.concurrent.Callable; import java.util.logging.Level; @@ -44,6 +45,7 @@ public class RetryHelper { private static final Logger log = Logger.getLogger(RetryHelper.class.getName()); + private final static boolean DEFAULT_RETRY_ON_TIMEOUTS = true; private final Stopwatch stopwatch; private final Callable callable; @@ -51,7 +53,6 @@ public class RetryHelper { private final ExceptionHandler exceptionHandler; private int attemptNumber; - private static final ThreadLocal context = new ThreadLocal<>(); public static class RetryHelperException extends RuntimeException { @@ -172,7 +173,7 @@ public String toString() { return toStringHelper.toString(); } - private V doRetry() throws RetryHelperException { + private V doRetry(boolean retryOnTimeout) throws RetryHelperException { stopwatch.start(); while (true) { attemptNumber++; @@ -189,7 +190,8 @@ private V doRetry() throws RetryHelperException { } exception = e; } catch (Exception e) { - if (!exceptionHandler.shouldRetry(e)) { + if (!exceptionHandler.shouldRetry(e) + || !retryOnTimeout && e.getCause() instanceof SocketTimeoutException) { throw new NonRetriableException(e); } exception = e; @@ -229,22 +231,30 @@ private static long getExponentialValue(long initialDelay, double backoffFactor, public static V runWithRetries(Callable callable) throws RetryHelperException { return runWithRetries(callable, RetryParams.defaultInstance(), - ExceptionHandler.defaultInstance()); + ExceptionHandler.defaultInstance(), DEFAULT_RETRY_ON_TIMEOUTS); } public static V runWithRetries(Callable callable, RetryParams params, ExceptionHandler exceptionHandler) throws RetryHelperException { - return runWithRetries(callable, params, exceptionHandler, Stopwatch.createUnstarted()); + return runWithRetries(callable, params, exceptionHandler, Stopwatch.createUnstarted(), + DEFAULT_RETRY_ON_TIMEOUTS); + } + + public static V runWithRetries(Callable callable, RetryParams params, + ExceptionHandler exceptionHandler, boolean retryOnTimeout) throws RetryHelperException { + return runWithRetries(callable, params, exceptionHandler, Stopwatch.createUnstarted(), + retryOnTimeout); } @VisibleForTesting static V runWithRetries(Callable callable, RetryParams params, - ExceptionHandler exceptionHandler, Stopwatch stopwatch) throws RetryHelperException { + ExceptionHandler exceptionHandler, Stopwatch stopwatch, boolean retryOnTimeout) + throws RetryHelperException { RetryHelper retryHelper = new RetryHelper<>(callable, params, exceptionHandler, stopwatch); Context previousContext = getContext(); setContext(new Context(retryHelper)); try { - return retryHelper.doRetry(); + return retryHelper.doRetry(retryOnTimeout); } finally { setContext(previousContext); } diff --git a/gcloud-java-core/src/test/java/com/google/gcloud/RetryHelperTest.java b/gcloud-java-core/src/test/java/com/google/gcloud/RetryHelperTest.java index 9a7cc2104f4a..e97b9deeeb9c 100644 --- a/gcloud-java-core/src/test/java/com/google/gcloud/RetryHelperTest.java +++ b/gcloud-java-core/src/test/java/com/google/gcloud/RetryHelperTest.java @@ -30,6 +30,7 @@ import org.junit.Test; import java.io.IOException; +import java.net.SocketTimeoutException; import java.util.Arrays; import java.util.Iterator; import java.util.concurrent.Callable; @@ -194,7 +195,7 @@ public void testTriesNoMoreLongerThanTotalRetryPeriod() { } throw new RuntimeException(); } - }), params, handler, stopwatch); + }), params, handler, stopwatch, true); fail(); } catch (RetriesExhaustedException expected) { // verify timesCalled @@ -202,6 +203,32 @@ public void testTriesNoMoreLongerThanTotalRetryPeriod() { } } + @Test + public void testNoRetriesOnSocketTimeout() { + final FakeTicker ticker = new FakeTicker(); + Stopwatch stopwatch = Stopwatch.createUnstarted(ticker); + RetryParams params = RetryParams.builder().initialRetryDelayMillis(0) + .totalRetryPeriodMillis(999) + .retryMinAttempts(5) + .retryMaxAttempts(10) + .build(); + ExceptionHandler handler = ExceptionHandler.builder().retryOn(RuntimeException.class).build(); + final AtomicInteger timesCalled = new AtomicInteger(0); + try { + RetryHelper.runWithRetries(callable(new Runnable() { + @Override public void run() { + timesCalled.incrementAndGet(); + throw new RetryHelper.RetryHelperException( + new SocketTimeoutException("Simulated socket timeout")); + } + }), params, handler, stopwatch, false); + fail(); + } catch (RetryHelper.RetryHelperException expected) { + // verify timesCalled + assertEquals(1, timesCalled.get()); + } + } + @Test public void testBackoffIsExponential() { // Total retry period set to 60 seconds so as to not factor into test diff --git a/gcloud-java-dns/src/main/java/com/google/gcloud/dns/DnsImpl.java b/gcloud-java-dns/src/main/java/com/google/gcloud/dns/DnsImpl.java index 3218daa543b2..42557199e3b0 100644 --- a/gcloud-java-dns/src/main/java/com/google/gcloud/dns/DnsImpl.java +++ b/gcloud-java-dns/src/main/java/com/google/gcloud/dns/DnsImpl.java @@ -215,7 +215,7 @@ public Zone create(final ZoneInfo zoneInfo, Dns.ZoneOption... options) { public ManagedZone call() { return dnsRpc.create(zoneInfo.toPb(), optionsMap); } - }, options().retryParams(), EXCEPTION_HANDLER); + }, options().retryParams(), EXCEPTION_HANDLER, false); return answer == null ? null : Zone.fromPb(this, answer); } catch (RetryHelper.RetryHelperException ex) { throw DnsException.translateAndThrow(ex); @@ -247,7 +247,7 @@ public boolean delete(final String zoneName) { public Boolean call() { return dnsRpc.deleteZone(zoneName); } - }, options().retryParams(), EXCEPTION_HANDLER); + }, options().retryParams(), EXCEPTION_HANDLER, false); } catch (RetryHelper.RetryHelperException ex) { throw DnsException.translateAndThrow(ex); } @@ -281,7 +281,7 @@ public ChangeRequest applyChangeRequest(final String zoneName, public Change call() { return dnsRpc.applyChangeRequest(zoneName, changeRequest.toPb(), optionsMap); } - }, options().retryParams(), EXCEPTION_HANDLER); + }, options().retryParams(), EXCEPTION_HANDLER, false); return answer == null ? null : ChangeRequest.fromPb(this, zoneName, answer); // not null } catch (RetryHelper.RetryHelperException ex) { throw DnsException.translateAndThrow(ex);