RetryHelper can be configured to not retry on socket timeouts.

This fixes #816 and #808.

Author: mderka

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,14 +45,14 @@
 public class RetryHelper<V> {
 
   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<V> callable;
   private final RetryParams params;
   private final ExceptionHandler exceptionHandler;
   private int attemptNumber;
 
-
   private static final ThreadLocal<Context> 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> V runWithRetries(Callable<V> callable) throws RetryHelperException {
     return runWithRetries(callable, RetryParams.defaultInstance(),
-        ExceptionHandler.defaultInstance());
+        ExceptionHandler.defaultInstance(), DEFAULT_RETRY_ON_TIMEOUTS);
   }
 
   public static <V> V runWithRetries(Callable<V> 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> V runWithRetries(Callable<V> callable, RetryParams params,
+      ExceptionHandler exceptionHandler, boolean retryOnTimeout) throws RetryHelperException {
+    return runWithRetries(callable, params, exceptionHandler, Stopwatch.createUnstarted(),
+        retryOnTimeout);
   }
 
   @VisibleForTesting
   static <V> V runWithRetries(Callable<V> callable, RetryParams params,
-      ExceptionHandler exceptionHandler, Stopwatch stopwatch) throws RetryHelperException {
+      ExceptionHandler exceptionHandler, Stopwatch stopwatch, boolean retryOnTimeout)
+      throws RetryHelperException {
     RetryHelper<V> 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);
     }

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<Void>() {
+      runWithRetries(new Callable<Void>() {
         @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<? extends E1Exception> exceptions = Arrays.asList(
         new E1Exception(), new E2Exception(), new E4Exception(), new E3Exception()).iterator();
     try {
-      RetryHelper.runWithRetries(new Callable<Void>() {
+      runWithRetries(new Callable<Void>() {
         @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<Integer>() {
+    int attempted = runWithRetries(new Callable<Integer>() {
       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,22 +188,47 @@ 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) {
             ticker.advance(1000, TimeUnit.MILLISECONDS);
           }
           throw new RuntimeException();
         }
-      }), params, handler, stopwatch);
+      }), params, handler, stopwatch, true);
       fail();
     } catch (RetriesExhaustedException expected) {
       // verify timesCalled
       assertEquals(sleepOnAttempt, timesCalled.get());
     }
   }
 
+  @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<Integer>() {
+    return runWithRetries(new Callable<Integer>() {
       @Override
       public Integer call() throws IOException {
         if (RetryHelper.getContext().getAttemptNumber() < retries) {

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);

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 {