RetryHelper can be configured to not retry on socket timeouts.

This fixes #816 and #808.
googleapis · Apr 8, 2016 · 03e1eaf · 03e1eaf
1 parent b29df66
commit 03e1eaf
Show file tree

Hide file tree

Showing 3 changed files with 48 additions and 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
@@ -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);
     }

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
@@ -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,14 +195,40 @@ public void testTriesNoMoreLongerThanTotalRetryPeriod() {
           }
           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 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
@@ -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);