Skip to content

RetryHelper can be configured to not retry on socket timeouts. #882

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 17 additions & 7 deletions gcloud-java-core/src/main/java/com/google/gcloud/RetryHelper.java
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
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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 {

This comment was marked as spam.

RetryHelper.runWithRetries(callable(new Runnable() {
@Override public void run() {
timesCalled.incrementAndGet();

This comment was marked as spam.

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
Expand Down
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