From 94459ba6e3a3fc642a6931ae6847081a126d1c5b Mon Sep 17 00:00:00 2001 From: Jeremy Daggett Date: Wed, 14 May 2014 14:39:10 -0700 Subject: [PATCH] Update openstack-keystone RetryOnRenew to handle 408 errors with a BackoffLimitedRetryHandler --- .../keystone/v2_0/handlers/RetryOnRenew.java | 27 ++++++++++---- .../v2_0/handlers/RetryOnRenewTest.java | 37 ++++++++++++++++++- 2 files changed, 54 insertions(+), 10 deletions(-) diff --git a/apis/openstack-keystone/src/main/java/org/jclouds/openstack/keystone/v2_0/handlers/RetryOnRenew.java b/apis/openstack-keystone/src/main/java/org/jclouds/openstack/keystone/v2_0/handlers/RetryOnRenew.java index 77646a972f..bc7b881200 100644 --- a/apis/openstack-keystone/src/main/java/org/jclouds/openstack/keystone/v2_0/handlers/RetryOnRenew.java +++ b/apis/openstack-keystone/src/main/java/org/jclouds/openstack/keystone/v2_0/handlers/RetryOnRenew.java @@ -22,11 +22,14 @@ import static org.jclouds.http.HttpUtils.releasePayload; import java.util.concurrent.TimeUnit; import javax.annotation.Resource; +import javax.inject.Named; +import org.jclouds.Constants; import org.jclouds.domain.Credentials; import org.jclouds.http.HttpCommand; import org.jclouds.http.HttpResponse; import org.jclouds.http.HttpRetryHandler; +import org.jclouds.http.handlers.BackoffLimitedRetryHandler; import org.jclouds.logging.Logger; import org.jclouds.openstack.keystone.v2_0.domain.Access; import org.jclouds.openstack.v2_0.reference.AuthHeaders; @@ -43,8 +46,6 @@ import com.google.inject.Singleton; /** * This will parse and set an appropriate exception on the command object. * - * @author Adrian Cole - * @author Zack Shoylev */ @Singleton public class RetryOnRenew implements HttpRetryHandler { @@ -52,13 +53,19 @@ public class RetryOnRenew implements HttpRetryHandler { protected Logger logger = Logger.NULL; @VisibleForTesting + @Inject(optional = true) + @Named(Constants.PROPERTY_MAX_RETRIES) static final int NUM_RETRIES = 5; private final LoadingCache authenticationResponseCache; + private final BackoffLimitedRetryHandler backoffHandler; + @Inject - protected RetryOnRenew(LoadingCache authenticationResponseCache) { + protected RetryOnRenew(LoadingCache authenticationResponseCache, + BackoffLimitedRetryHandler backoffHandler) { this.authenticationResponseCache = authenticationResponseCache; + this.backoffHandler = backoffHandler; } /* @@ -73,6 +80,7 @@ public class RetryOnRenew implements HttpRetryHandler { @Override public boolean shouldRetryRequest(HttpCommand command, HttpResponse response) { + boolean retry = false; // default try { switch (response.getStatusCode()) { case 401: @@ -80,7 +88,7 @@ public class RetryOnRenew implements HttpRetryHandler { Multimap headers = command.getCurrentRequest().getHeaders(); if (headers != null && headers.containsKey(AuthHeaders.AUTH_USER) && headers.containsKey(AuthHeaders.AUTH_KEY) && !headers.containsKey(AuthHeaders.AUTH_TOKEN)) { - return false; + retry = false; } else { closeClientButKeepContentStream(response); // This is not an authentication request returning 401 @@ -92,12 +100,12 @@ public class RetryOnRenew implements HttpRetryHandler { logger.debug("invalidating authentication token - first time for %s", command); retryCountMap.put(command, 1); authenticationResponseCache.invalidateAll(); - return true; + retry = true; } else { // This request has failed before if (count + 1 >= NUM_RETRIES) { logger.debug("too many 401s - giving up after: %s for %s", count, command); - return false; + retry = false; } else { // Retry just in case logger.debug("invalidating authentication token - retry %s for %s", count, command); @@ -105,12 +113,15 @@ public class RetryOnRenew implements HttpRetryHandler { // Wait between retries authenticationResponseCache.invalidateAll(); Uninterruptibles.sleepUninterruptibly(5, TimeUnit.SECONDS); - return true; + retry = true; } } } + break; + case 408: + return backoffHandler.shouldRetryRequest(command, response); } - return false; + return retry; } finally { releasePayload(response); } diff --git a/apis/openstack-keystone/src/test/java/org/jclouds/openstack/keystone/v2_0/handlers/RetryOnRenewTest.java b/apis/openstack-keystone/src/test/java/org/jclouds/openstack/keystone/v2_0/handlers/RetryOnRenewTest.java index 17e687f1ef..0a812bbc54 100644 --- a/apis/openstack-keystone/src/test/java/org/jclouds/openstack/keystone/v2_0/handlers/RetryOnRenewTest.java +++ b/apis/openstack-keystone/src/test/java/org/jclouds/openstack/keystone/v2_0/handlers/RetryOnRenewTest.java @@ -28,6 +28,7 @@ import org.jclouds.domain.Credentials; import org.jclouds.http.HttpCommand; import org.jclouds.http.HttpRequest; import org.jclouds.http.HttpResponse; +import org.jclouds.http.handlers.BackoffLimitedRetryHandler; import org.jclouds.io.Payloads; import org.jclouds.openstack.keystone.v2_0.domain.Access; import org.testng.annotations.Test; @@ -48,6 +49,7 @@ public class RetryOnRenewTest { HttpResponse response = createMock(HttpResponse.class); @SuppressWarnings("unchecked") LoadingCache cache = createMock(LoadingCache.class); + BackoffLimitedRetryHandler backoffHandler = createMock(BackoffLimitedRetryHandler.class); expect(command.getCurrentRequest()).andReturn(request); @@ -60,8 +62,9 @@ public class RetryOnRenewTest { replay(command); replay(response); replay(cache); + replay(backoffHandler); - RetryOnRenew retry = new RetryOnRenew(cache); + RetryOnRenew retry = new RetryOnRenew(cache, backoffHandler); assertTrue(retry.shouldRetryRequest(command, response)); @@ -69,6 +72,7 @@ public class RetryOnRenewTest { verify(response); verify(cache); } + @Test public void test401ShouldRetry4Times() { HttpCommand command = createMock(HttpCommand.class); @@ -77,6 +81,7 @@ public class RetryOnRenewTest { @SuppressWarnings("unchecked") LoadingCache cache = createMock(LoadingCache.class); + BackoffLimitedRetryHandler backoffHandler = createMock(BackoffLimitedRetryHandler.class); expect(command.getCurrentRequest()).andReturn(request).anyTimes(); expect(request.getHeaders()).andStubReturn(null); @@ -89,7 +94,7 @@ public class RetryOnRenewTest { replay(command, request, response, cache); - RetryOnRenew retry = new RetryOnRenew(cache); + RetryOnRenew retry = new RetryOnRenew(cache, backoffHandler); for (int i = 0; i < RetryOnRenew.NUM_RETRIES - 1; ++i) { assertTrue(retry.shouldRetryRequest(command, response), "Expected retry to succeed"); @@ -99,4 +104,32 @@ public class RetryOnRenewTest { verify(command, response, cache); } + + @Test + public void test408ShouldRetry() { + HttpCommand command = createMock(HttpCommand.class); + HttpResponse response = createMock(HttpResponse.class); + @SuppressWarnings("unchecked") + LoadingCache cache = createMock(LoadingCache.class); + BackoffLimitedRetryHandler backoffHandler = createMock(BackoffLimitedRetryHandler.class); + + expect(response.getPayload()).andReturn(Payloads.newStringPayload( + "The server has waited too long for the request to be sent by the client.")).times(2); + expect(backoffHandler.shouldRetryRequest(command, response)).andReturn(true).once(); + expect(response.getStatusCode()).andReturn(408).once(); + + replay(command); + replay(response); + replay(cache); + replay(backoffHandler); + + RetryOnRenew retry = new RetryOnRenew(cache, backoffHandler); + + assertTrue(retry.shouldRetryRequest(command, response)); + + verify(command); + verify(response); + verify(cache); + verify(backoffHandler); + } }