Merge pull request #1319 from jclouds/respectPROPERTY_MAX_REDIRECTS

RedirectionRetryHandler ignored PROPERTY_MAX_REDIRECTS on backoff

apis/deltacloud/src/test/java/org/jclouds/deltacloud/handlers/DeltacloudRedirectionRetryHandlerTest.java

@@ -67,6 +67,7 @@ public class DeltacloudRedirectionRetryHandlerTest {
       HttpRequest request = HttpRequest.builder().method("GET").endpoint("http://localhost").build();
       HttpResponse response = HttpResponse.builder().statusCode(302).message("HTTP/1.1 302 Found").build();
 
+      expect(command.isReplayable()).andReturn(true);
       expect(command.getCurrentRequest()).andReturn(request).atLeastOnce();
       expect(command.incrementRedirectCount()).andReturn(1);
 

core/src/main/java/org/jclouds/http/handlers/RedirectionRetryHandler.java

@@ -35,13 +35,12 @@ import org.jclouds.http.HttpRequest;
 import org.jclouds.http.HttpResponse;
 import org.jclouds.http.HttpRetryHandler;
 import org.jclouds.logging.Logger;
-import org.jclouds.util.Multimaps2;
 
-import com.google.common.collect.Multimap;
 import com.google.inject.Inject;
 
 /**
- * Handles Retryable responses with error codes in the 3xx range
+ * Handles Retryable responses with error codes in the 3xx range, backing off
+ * when redirecting to itself.
  * 
  * @author Adrian Cole
  */
@@ -63,35 +62,36 @@ public class RedirectionRetryHandler implements HttpRetryHandler {
 
    public boolean shouldRetryRequest(HttpCommand command, HttpResponse response) {
       closeClientButKeepContentStream(response);
-      String hostHeader = response.getFirstHeaderOrNull(LOCATION);
+      if (!command.isReplayable()) {
-      if (command.incrementRedirectCount() < retryCountLimit && hostHeader != null) {
+         logger.error("Cannot retry after redirect, command is not replayable: %s", command);
-         URI redirectionUrl = URI.create(hostHeader);
-
-         // if you are sent the same uri, assume there's a transient problem and retry.
-         HttpRequest currentRequest = command.getCurrentRequest();
-         if (redirectionUrl.equals(currentRequest.getEndpoint()))
-            return backoffHandler.shouldRetryRequest(command, response);
-
-         assert redirectionUrl.getPath() != null : "no path in redirect header from: " + response;
-         if (!redirectionUrl.isAbsolute()) {
-            redirectionUrl = uriBuilder(currentRequest.getEndpoint()).path(redirectionUrl.getPath())
-                  .query(redirectionUrl.getQuery()).build();
-         }
-
-         if (currentRequest.getFirstHeaderOrNull(HOST) != null && redirectionUrl.getHost() != null) {
-            String host = redirectionUrl.getHost();
-            if (redirectionUrl.getPort() != -1) {
-               host += ":" + redirectionUrl.getPort();
-            }
-            Multimap<String, String> newHeaders = Multimaps2.replaceValue(currentRequest.getHeaders(), HOST, host);
-            command.setCurrentRequest(currentRequest.toBuilder().headers(newHeaders).endpoint(redirectionUrl).build());
-         } else {
-            command.setCurrentRequest(currentRequest.toBuilder().endpoint(redirectionUrl).build());
-         }
-         return true;
-      } else {
          return false;
       }
+      if (command.incrementRedirectCount() > retryCountLimit) {
+         logger.error("Cannot retry after redirect, command exceeded retry limit %d: %s", retryCountLimit, command);
+         return false;
+      }
+      String location = response.getFirstHeaderOrNull(LOCATION);
+      if (location == null) {
+         logger.error("Cannot retry after redirect, no host header: %s", command);
+         return false;
+      }
+      HttpRequest current = command.getCurrentRequest();
+      URI redirect = URI.create(location);
+      if (!redirect.isAbsolute()) {
+         if (redirect.getPath() == null) {
+            logger.error("Cannot retry after redirect, no path in location header %s", command);
+            return false;
+         }
+         redirect = uriBuilder(current.getEndpoint()).path(redirect.getPath()).query(redirect.getQuery()).build();
+      }
+      if (redirect.equals(current.getEndpoint())) {
+         backoffHandler.imposeBackoffExponentialDelay(command.getRedirectCount(), "redirect: " + command.toString());
+      } else if (current.getFirstHeaderOrNull(HOST) != null && redirect.getHost() != null) {
+         String host = redirect.getPort() > 0 ? redirect.getHost() + ":" + redirect.getPort() : redirect.getHost();
+         command.setCurrentRequest(current.toBuilder().replaceHeader(HOST, host).endpoint(redirect).build());
+      } else {
+         command.setCurrentRequest(current.toBuilder().endpoint(redirect).build());
+      }
+      return true;
    }
-
 }

core/src/test/java/org/jclouds/http/handlers/RedirectionRetryHandlerTest.java

@@ -59,6 +59,7 @@ public class RedirectionRetryHandlerTest {
                                           .statusCode(302)
                                           .message("HTTP/1.1 302 Found").build();
 
+      expect(command.isReplayable()).andReturn(true);
       expect(command.incrementRedirectCount()).andReturn(0);
 
       replay(command);
@@ -80,7 +81,8 @@ public class RedirectionRetryHandlerTest {
                                           .message("HTTP/1.1 302 Found")
                                           .addHeader(LOCATION, "/api/v0.8b-ext2.5/Error.aspx?aspxerrorpath=/api/v0.8b-ext2.5/org.svc/1906645").build(); 
 
-      expect(command.incrementRedirectCount()).andReturn(5);
+      expect(command.isReplayable()).andReturn(true);
+      expect(command.incrementRedirectCount()).andReturn(6);
 
       replay(command);
 
@@ -174,6 +176,7 @@ public class RedirectionRetryHandlerTest {
    protected void verifyRedirectRoutes(HttpRequest request, HttpResponse response, HttpRequest expected) {
       HttpCommand command = createMock(HttpCommand.class);
 
+      expect(command.isReplayable()).andReturn(true);
       expect(command.incrementRedirectCount()).andReturn(0);
       expect(command.getCurrentRequest()).andReturn(request);
       command.setCurrentRequest(expected);