Skip to content

Commit

Permalink
RetryHelper can be configured to not retry on socket timeouts.
Browse files Browse the repository at this point in the history 
This fixes googleapis#816 and googleapis#808.
  • Loading branch information
@mderka
mderka committed Apr 8, 2016
1 parent b29df66 commit d976ed4
Show file tree
Hide file tree
Showing 4 changed files with 56 additions and 17 deletions.
24 changes: 17 additions & 7 deletions gcloud-java-core/src/main/java/com/google/gcloud/RetryHelper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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 {
Expand Down Expand Up @@ -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++;
Expand All @@ -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;
Expand Down Expand Up @@ -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);
}
Expand Down
41 changes: 34 additions & 7 deletions gcloud-java-core/src/test/java/com/google/gcloud/RetryHelperTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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());
Expand All @@ -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();
}
Expand All @@ -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++;
Expand All @@ -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) {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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) {
Expand Down
6 changes: 3 additions & 3 deletions gcloud-java-dns/src/main/java/com/google/gcloud/dns/DnsImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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);
Expand Down
2 changes: 2 additions & 0 deletions gcloud-java-dns/src/test/java/com/google/gcloud/dns/DnsImplTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -37,6 +38,7 @@
import org.junit.Before;
import org.junit.Test;

import java.net.SocketTimeoutException;
import java.util.Map;

public class DnsImplTest {
Expand Down

0 comments on commit d976ed4

Please sign in to comment.