Session renew should fail after a limited number of failed attempts

apis/cloudstack/src/main/java/org/jclouds/cloudstack/config/CloudStackRestClientModule.java

@@ -122,7 +122,7 @@ import org.jclouds.cloudstack.filters.AuthenticationFilter;
 import org.jclouds.cloudstack.filters.QuerySigner;
 import org.jclouds.cloudstack.functions.LoginWithPasswordCredentials;
 import org.jclouds.cloudstack.handlers.CloudStackErrorHandler;
-import org.jclouds.cloudstack.handlers.RetryOnRenewAndLogoutOnClose;
+import org.jclouds.cloudstack.handlers.InvalidateSessionAndRetryOn401AndLogoutOnClose;
 import org.jclouds.concurrent.RetryOnTimeOutExceptionFunction;
 import org.jclouds.domain.Credentials;
 import org.jclouds.http.HttpErrorHandler;
@@ -242,7 +242,7 @@ public class CloudStackRestClientModule extends RestClientModule<CloudStackClien
       
       // session client is used directly for filters and retry handlers, so let's bind it explicitly
       bindClientAndAsyncClient(binder(), SessionClient.class, SessionAsyncClient.class);
-      bind(HttpRetryHandler.class).annotatedWith(ClientError.class).to(RetryOnRenewAndLogoutOnClose.class);
+      bind(HttpRetryHandler.class).annotatedWith(ClientError.class).to(InvalidateSessionAndRetryOn401AndLogoutOnClose.class);
       
       super.configure();
    }

apis/cloudstack/src/main/java/org/jclouds/cloudstack/handlers/InvalidateSessionAndRetryOn401AndLogoutOnClose.java

@@ -26,7 +26,7 @@ import org.jclouds.cloudstack.features.SessionClient;
 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 javax.annotation.PreDestroy;
@@ -36,12 +36,11 @@ import static org.jclouds.http.HttpUtils.releasePayload;
 
 /**
  * This will parse and set an appropriate exception on the command object.
- * 
+ *
  * @author Adrian Cole
- * 
  */
 @Singleton
-public class RetryOnRenewAndLogoutOnClose implements HttpRetryHandler {
+public class InvalidateSessionAndRetryOn401AndLogoutOnClose extends BackoffLimitedRetryHandler {
    @Resource
    protected Logger logger = Logger.NULL;
 
@@ -49,8 +48,8 @@ public class RetryOnRenewAndLogoutOnClose implements HttpRetryHandler {
    private final SessionClient sessionClient;
 
    @Inject
-   protected RetryOnRenewAndLogoutOnClose(LoadingCache<Credentials, LoginResponse> authenticationResponseCache,
+   protected InvalidateSessionAndRetryOn401AndLogoutOnClose(LoadingCache<Credentials, LoginResponse> authenticationResponseCache,
-            SessionClient sessionClient) {
+                                                            SessionClient sessionClient) {
       this.authenticationResponseCache = authenticationResponseCache;
       this.sessionClient = sessionClient;
    }
@@ -61,7 +60,7 @@ public class RetryOnRenewAndLogoutOnClose implements HttpRetryHandler {
          switch (response.getStatusCode()) {
             case 401:
                authenticationResponseCache.invalidateAll();
-               return true;
+               return super.shouldRetryRequest(command, response);
          }
          return false;
 

apis/cloudstack/src/test/java/org/jclouds/cloudstack/handlers/CloudStackErrorHandlerTest.java

@@ -18,14 +18,7 @@
  */
 package org.jclouds.cloudstack.handlers;
 
-import static org.easymock.EasyMock.expect;
+import com.google.inject.Guice;
-import static org.easymock.EasyMock.reportMatcher;
-import static org.easymock.classextension.EasyMock.createMock;
-import static org.easymock.classextension.EasyMock.replay;
-import static org.easymock.classextension.EasyMock.verify;
-
-import java.net.URI;
-
 import org.easymock.IArgumentMatcher;
 import org.jclouds.http.HttpCommand;
 import org.jclouds.http.HttpRequest;
@@ -36,54 +29,59 @@ import org.jclouds.rest.ResourceNotFoundException;
 import org.jclouds.util.Strings2;
 import org.testng.annotations.Test;
 
-import com.google.inject.Guice;
+import java.net.URI;
+
+import static org.easymock.EasyMock.expect;
+import static org.easymock.EasyMock.reportMatcher;
+import static org.easymock.classextension.EasyMock.createMock;
+import static org.easymock.classextension.EasyMock.replay;
+import static org.easymock.classextension.EasyMock.verify;
 
 /**
- * 
  * @author Adrian Cole
  */
-@Test(groups = { "unit" })
+@Test(groups = {"unit"})
 public class CloudStackErrorHandlerTest {
 
    @Test
    public void test400MakesIllegalArgumentException() {
       assertCodeMakes("GET", URI.create("https://cloudstack.com/foo"), 400, "", "Bad Request",
-            IllegalArgumentException.class);
+         IllegalArgumentException.class);
    }
 
    @Test
    public void test401MakesAuthorizationException() {
       assertCodeMakes("GET", URI.create("https://cloudstack.com/foo"), 401, "", "Unauthorized",
-            AuthorizationException.class);
+         AuthorizationException.class);
    }
 
    @Test
    public void test404MakesResourceNotFoundException() {
       assertCodeMakes("GET", URI.create("https://cloudstack.com/foo"), 404, "", "Not Found",
-            ResourceNotFoundException.class);
+         ResourceNotFoundException.class);
    }
 
    @Test
    public void test405MakesIllegalArgumentException() {
       assertCodeMakes("GET", URI.create("https://cloudstack.com/foo"), 405, "", "Method Not Allowed",
-            IllegalArgumentException.class);
+         IllegalArgumentException.class);
    }
 
    @Test
    public void test431MakesIllegalStateException() {
       assertCodeMakes("GET", URI.create("https://cloudstack.com/foo"), 431, "", "Method Not Allowed",
-            IllegalStateException.class);
+         IllegalStateException.class);
    }
 
    @Test
    public void test431MakesResourceNotFoundExceptionOnDelete() {
       assertCodeMakes(
-            "GET",
+         "GET",
-            URI.create("https://api.ninefold.com/compute/v1.0/?response=json&command=deleteSSHKeyPair"),
+         URI.create("https://api.ninefold.com/compute/v1.0/?response=json&command=deleteSSHKeyPair"),
-            431,
+         431,
-            "",
+         "",
-            "{ \"deletekeypairresponse\" : {\"errorcode\" : 431, \"errortext\" : \"A key pair with name 'adriancole-adapter-test-keypair' does not exist for account jclouds in domain id=457\"}  }",
+         "{ \"deletekeypairresponse\" : {\"errorcode\" : 431, \"errortext\" : \"A key pair with name 'adriancole-adapter-test-keypair' does not exist for account jclouds in domain id=457\"}  }",
-            ResourceNotFoundException.class);
+         ResourceNotFoundException.class);
    }
 
    @Test
@@ -91,30 +89,37 @@ public class CloudStackErrorHandlerTest {
       assertCodeMakes("GET", URI.create("https://cloudstack.com/foo"), 409, "", "Conflict", IllegalStateException.class);
    }
 
-   @Test void test537MakesIllegalStateException() {
+   @Test
+   public void test531MakesAuthorizationException() {
+      assertCodeMakes("GET", URI.create("https://cloudstack.com/foo"), 531, "", "Unauthoized",
+         AuthorizationException.class);
+   }
+
+   @Test
+   void test537MakesIllegalStateException() {
       assertCodeMakes(
-            "GET",
+         "GET",
-            URI.create("http://10.26.26.155:8080/client/api?response=json&command=createIpForwardingRule&ipaddressid=37&startport=22&protocol=tcp"),
+         URI.create("http://10.26.26.155:8080/client/api?response=json&command=createIpForwardingRule&ipaddressid=37&startport=22&protocol=tcp"),
-            537,
+         537,
-            "",
+         "",
-            "{ \"createipforwardingruleresponse\" : {\"errorcode\" : 537, \"errortext\" : \"There is already firewall rule specified for the ip address id=37\"}  }",
+         "{ \"createipforwardingruleresponse\" : {\"errorcode\" : 537, \"errortext\" : \"There is already firewall rule specified for the ip address id=37\"}  }",
-            IllegalStateException.class);
+         IllegalStateException.class);
    }
 
    private void assertCodeMakes(String method, URI uri, int statusCode, String message, String content,
-         Class<? extends Exception> expected) {
+                                Class<? extends Exception> expected) {
       assertCodeMakes(method, uri, statusCode, message, "text/xml", content, expected);
    }
 
    private void assertCodeMakes(String method, URI uri, int statusCode, String message, String contentType,
-         String content, Class<? extends Exception> expected) {
+                                String content, Class<? extends Exception> expected) {
 
       CloudStackErrorHandler function = Guice.createInjector().getInstance(CloudStackErrorHandler.class);
 
       HttpCommand command = createMock(HttpCommand.class);
       HttpRequest request = new HttpRequest(method, uri);
       HttpResponse response = new HttpResponse(statusCode, message, Payloads.newInputStreamPayload(Strings2
-            .toInputStream(content)));
+         .toInputStream(content)));
       response.getPayload().getContentMetadata().setContentType(contentType);
 
       expect(command.getCurrentRequest()).andReturn(request).atLeastOnce();

apis/cloudstack/src/test/java/org/jclouds/cloudstack/handlers/InvalidateSessionAndRetryOn401AndLogoutOnCloseTest.java

@@ -19,6 +19,7 @@
 package org.jclouds.cloudstack.handlers;
 
 import com.google.common.cache.LoadingCache;
+import org.easymock.IAnswer;
 import org.jclouds.cloudstack.domain.LoginResponse;
 import org.jclouds.cloudstack.features.SessionClient;
 import org.jclouds.domain.Credentials;
@@ -27,37 +28,61 @@ import org.jclouds.http.HttpResponse;
 import org.jclouds.io.Payloads;
 import org.testng.annotations.Test;
 
+import java.util.concurrent.atomic.AtomicInteger;
+
 import static org.easymock.EasyMock.createMock;
+import static org.easymock.EasyMock.expect;
 import static org.easymock.EasyMock.expectLastCall;
 import static org.easymock.EasyMock.replay;
 import static org.easymock.EasyMock.verify;
+import static org.testng.Assert.assertFalse;
 import static org.testng.Assert.assertTrue;
 
 /**
- * Tests behavior of {@code RetryOnRenewAndLogoutOnClose} handler
+ * Tests behavior of {@code InvalidateSessionAndRetryOn401AndLogoutOnClose} handler
  *
  * @author grkvlt@apache.org
  */
-@Test(groups = "unit", testName = "RetryOnRenewAndLogoutOnCloseTest")
+@Test(groups = "unit", testName = "InvalidateSessionAndRetryOn401AndLogoutOnCloseTest")
-public class RetryOnRenewAndLogoutOnCloseTest {
+public class InvalidateSessionAndRetryOn401AndLogoutOnCloseTest {
+
    @SuppressWarnings("unchecked")
    @Test
-   public void test401ShouldRetry() {
+   public void test401ShouldRetryAndFailAfterFiveAttempts() {
       HttpCommand command = createMock(HttpCommand.class);
       SessionClient sessionClient = createMock(SessionClient.class);
       LoadingCache<Credentials, LoginResponse> cache = createMock(LoadingCache.class);
 
       cache.invalidateAll();
-      expectLastCall();
+      expectLastCall().anyTimes();
+
+      final AtomicInteger counter = new AtomicInteger();
+      expect(command.incrementFailureCount()).andAnswer(new IAnswer<Integer>() {
+         @Override
+         public Integer answer() throws Throwable {
+            return counter.incrementAndGet();
+         }
+      }).anyTimes();
+      expect(command.isReplayable()).andReturn(true).anyTimes();
+      expect(command.getFailureCount()).andAnswer(new IAnswer<Integer>() {
+         @Override
+         public Integer answer() throws Throwable {
+            return counter.get();
+         }
+      }).anyTimes();
 
       replay(cache, command);
 
       HttpResponse response = HttpResponse.builder().payload(
          Payloads.newStringPayload("Not relevant")).statusCode(401).build();
 
-      RetryOnRenewAndLogoutOnClose retry = new RetryOnRenewAndLogoutOnClose(cache, sessionClient);
+      InvalidateSessionAndRetryOn401AndLogoutOnClose retry =
+         new InvalidateSessionAndRetryOn401AndLogoutOnClose(cache, sessionClient);
 
-      assertTrue(retry.shouldRetryRequest(command, response));
+      for (int i = 0; i < 5; i++) {
+         assertTrue(retry.shouldRetryRequest(command, response));
+      }
+      assertFalse(retry.shouldRetryRequest(command, response));
 
       verify(cache, command);
    }