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..8d683e62d972 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..829020a664ca 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 @@ -16,6 +16,7 @@ package com.google.gcloud; +import static com.google.gcloud.RetryHelper.runWithRetries; import static java.util.concurrent.Executors.callable; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNull; @@ -30,6 +31,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; @@ -67,7 +69,7 @@ public void testTriesWithExceptionHandling() { .retryOn(IOException.class).abortOn(RuntimeException.class).build(); final AtomicInteger count = new AtomicInteger(3); try { - RetryHelper.runWithRetries(new Callable() { + runWithRetries(new Callable() { @Override public Void call() throws IOException, NullPointerException { if (count.decrementAndGet() == 2) { assertEquals(1, RetryHelper.getContext().getAttemptNumber()); @@ -91,7 +93,7 @@ public void testTriesWithExceptionHandling() { final Iterator exceptions = Arrays.asList( new E1Exception(), new E2Exception(), new E4Exception(), new E3Exception()).iterator(); try { - RetryHelper.runWithRetries(new Callable() { + runWithRetries(new Callable() { @Override public Void call() throws E1Exception { throw exceptions.next(); } @@ -113,7 +115,7 @@ public void testTriesAtLeastMinTimes() { .build(); final int timesToFail = 7; assertNull(RetryHelper.getContext()); - int attempted = RetryHelper.runWithRetries(new Callable() { + int attempted = runWithRetries(new Callable() { int timesCalled; @Override public Integer call() throws IOException { timesCalled++; @@ -140,7 +142,7 @@ public void testTriesNoMoreThanMaxTimes() { .build(); final AtomicInteger timesCalled = new AtomicInteger(0); try { - RetryHelper.runWithRetries(callable(new Runnable() { + runWithRetries(callable(new Runnable() { @Override public void run() { // Throw an exception up to maxAttempts times, should never be called beyond that if (timesCalled.incrementAndGet() <= maxAttempts) { @@ -186,7 +188,7 @@ public void testTriesNoMoreLongerThanTotalRetryPeriod() { final int sleepOnAttempt = 8; final AtomicInteger timesCalled = new AtomicInteger(0); try { - RetryHelper.runWithRetries(callable(new Runnable() { + runWithRetries(callable(new Runnable() { @Override public void run() { timesCalled.incrementAndGet(); if (timesCalled.get() == sleepOnAttempt) { @@ -194,7 +196,7 @@ public void testTriesNoMoreLongerThanTotalRetryPeriod() { } throw new RuntimeException(); } - }), params, handler, stopwatch); + }), params, handler, stopwatch, true); fail(); } catch (RetriesExhaustedException expected) { // verify timesCalled @@ -202,6 +204,31 @@ 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 RuntimeException(new SocketTimeoutException("Simulated socket timeout")); + } + }), params, handler, stopwatch, false); + fail(); + } catch (RuntimeException 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 @@ -248,7 +275,7 @@ private int invokeNested(final int level, final int retries) { if (level < 0) { return 0; } - return RetryHelper.runWithRetries(new Callable() { + return runWithRetries(new Callable() { @Override public Integer call() throws IOException { if (RetryHelper.getContext().getAttemptNumber() < retries) { 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); diff --git a/gcloud-java-dns/src/test/java/com/google/gcloud/dns/DnsImplTest.java b/gcloud-java-dns/src/test/java/com/google/gcloud/dns/DnsImplTest.java index c359871b998c..9cf5b0de58c2 100644 --- a/gcloud-java-dns/src/test/java/com/google/gcloud/dns/DnsImplTest.java +++ b/gcloud-java-dns/src/test/java/com/google/gcloud/dns/DnsImplTest.java @@ -26,6 +26,7 @@ import com.google.common.collect.ImmutableMap; import com.google.common.collect.Lists; import com.google.gcloud.Page; +import com.google.gcloud.RetryHelper; import com.google.gcloud.RetryParams; import com.google.gcloud.ServiceOptions; import com.google.gcloud.dns.spi.DnsRpc; @@ -37,6 +38,7 @@ import org.junit.Before; import org.junit.Test; +import java.net.SocketTimeoutException; import java.util.Map; public class DnsImplTest {