From 7167b473b47178a91cfdea19e236c84fcb94525f Mon Sep 17 00:00:00 2001 From: Andrea Turli Date: Wed, 26 Oct 2016 16:55:21 +0200 Subject: [PATCH] Fix retryOnRenew classes These classes should not close the release the payload as they are not reading it. - fix swift - fix openstack-swift v1 and v2 - fix RetryOnRenewTest for v1 and v2 --- .../keystone/v2_0/handlers/RetryOnRenew.java | 79 ++++++++----------- .../v2_0/handlers/RetryOnRenewTest.java | 2 - .../openstack/handlers/RetryOnRenew.java | 73 ++++++++--------- .../keystone/v1_1/handlers/RetryOnRenew.java | 77 ++++++++---------- .../v1_1/handlers/RetryOnRenewTest.java | 4 - 5 files changed, 100 insertions(+), 135 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 07983e9680..6364465f56 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 @@ -16,8 +16,6 @@ */ package org.jclouds.openstack.keystone.v2_0.handlers; -import static org.jclouds.http.HttpUtils.closeClientButKeepContentStream; - import java.util.concurrent.TimeUnit; import javax.annotation.Resource; @@ -79,54 +77,45 @@ public class RetryOnRenew implements HttpRetryHandler { @Override public boolean shouldRetryRequest(HttpCommand command, HttpResponse response) { boolean retry = false; // default - try { - switch (response.getStatusCode()) { - case 401: - // Do not retry on 401 from authentication request - Multimap headers = command.getCurrentRequest().getHeaders(); - if (headers != null && headers.containsKey(AuthHeaders.AUTH_USER) - && headers.containsKey(AuthHeaders.AUTH_KEY) && !headers.containsKey(AuthHeaders.AUTH_TOKEN)) { - retry = false; - } else { - closeClientButKeepContentStream(response); - // This is not an authentication request returning 401 - // Check if we already had seen this request - Integer count = retryCountMap.getIfPresent(command); + switch (response.getStatusCode()) { + case 401: + // Do not retry on 401 from authentication request + Multimap headers = command.getCurrentRequest().getHeaders(); + if (headers != null && headers.containsKey(AuthHeaders.AUTH_USER) + && headers.containsKey(AuthHeaders.AUTH_KEY) && !headers.containsKey(AuthHeaders.AUTH_TOKEN)) { + retry = false; + } else { + // This is not an authentication request returning 401 + // Check if we already had seen this request + Integer count = retryCountMap.getIfPresent(command); - if (count == null) { - // First time this non-authentication request failed - logger.debug("invalidating authentication token - first time for %s", command); - retryCountMap.put(command, 1); - authenticationResponseCache.invalidateAll(); - retry = true; + if (count == null) { + // First time this non-authentication request failed + logger.debug("invalidating authentication token - first time for %s", command); + retryCountMap.put(command, 1); + authenticationResponseCache.invalidateAll(); + 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); + retry = false; } else { - // This request has failed before - if (count + 1 >= NUM_RETRIES) { - logger.debug("too many 401s - giving up after: %s for %s", count, command); - retry = false; - } else { - // Retry just in case - logger.debug("invalidating authentication token - retry %s for %s", count, command); - retryCountMap.put(command, count + 1); - // Wait between retries - authenticationResponseCache.invalidateAll(); - Uninterruptibles.sleepUninterruptibly(5, TimeUnit.SECONDS); - retry = true; - } + // Retry just in case + logger.debug("invalidating authentication token - retry %s for %s", count, command); + retryCountMap.put(command, count + 1); + // Wait between retries + authenticationResponseCache.invalidateAll(); + Uninterruptibles.sleepUninterruptibly(5, TimeUnit.SECONDS); + retry = true; } } - break; - case 408: - return backoffHandler.shouldRetryRequest(command, response); - } - return retry; - } finally { - // If the request is failed and is not going to be retried, the - // ErrorHandler will be invoked and it might need to read the payload. - // For some kind of payload sources, such as the OkHttp Source, if the - // payload is released, the upcoming operations will fail. - closeClientButKeepContentStream(response); + } + break; + case 408: + return backoffHandler.shouldRetryRequest(command, response); } + return retry; } } 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 badf1203e6..840cda3d3e 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 @@ -111,8 +111,6 @@ public class RetryOnRenewTest { 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(3); expect(backoffHandler.shouldRetryRequest(command, response)).andReturn(true).once(); expect(response.getStatusCode()).andReturn(408).once(); diff --git a/common/openstack/src/main/java/org/jclouds/openstack/handlers/RetryOnRenew.java b/common/openstack/src/main/java/org/jclouds/openstack/handlers/RetryOnRenew.java index 1d71d7f038..19ce4a3736 100644 --- a/common/openstack/src/main/java/org/jclouds/openstack/handlers/RetryOnRenew.java +++ b/common/openstack/src/main/java/org/jclouds/openstack/handlers/RetryOnRenew.java @@ -16,9 +16,6 @@ */ package org.jclouds.openstack.handlers; -import static org.jclouds.http.HttpUtils.closeClientButKeepContentStream; -import static org.jclouds.http.HttpUtils.releasePayload; - import java.util.concurrent.TimeUnit; import javax.annotation.Resource; @@ -75,49 +72,43 @@ public class RetryOnRenew implements HttpRetryHandler { @Override public boolean shouldRetryRequest(HttpCommand command, HttpResponse response) { boolean retry = false; // default - try { - switch (response.getStatusCode()) { - case 401: - // Do not retry on 401 from authentication request - Multimap headers = command.getCurrentRequest().getHeaders(); - if (headers != null && headers.containsKey(AuthHeaders.AUTH_USER) - && headers.containsKey(AuthHeaders.AUTH_KEY) && !headers.containsKey(AuthHeaders.AUTH_TOKEN)) { - retry = false; - } else { - closeClientButKeepContentStream(response); - // This is not an authentication request returning 401 - // Check if we already had seen this request - Integer count = retryCountMap.getIfPresent(command); + switch (response.getStatusCode()) { + case 401: + // Do not retry on 401 from authentication request + Multimap headers = command.getCurrentRequest().getHeaders(); + if (headers != null && headers.containsKey(AuthHeaders.AUTH_USER) + && headers.containsKey(AuthHeaders.AUTH_KEY) && !headers.containsKey(AuthHeaders.AUTH_TOKEN)) { + retry = false; + } else { + // This is not an authentication request returning 401 + // Check if we already had seen this request + Integer count = retryCountMap.getIfPresent(command); - if (count == null) { - // First time this non-authentication request failed - logger.debug("invalidating authentication token - first time for %s", command); - retryCountMap.put(command, 1); - authenticationResponseCache.invalidateAll(); - retry = true; + if (count == null) { + // First time this non-authentication request failed + logger.debug("invalidating authentication token - first time for %s", command); + retryCountMap.put(command, 1); + authenticationResponseCache.invalidateAll(); + 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); + retry = false; } else { - // This request has failed before - if (count + 1 >= NUM_RETRIES) { - logger.debug("too many 401s - giving up after: %s for %s", count, command); - retry = false; - } else { - // Retry just in case - logger.debug("invalidating authentication token - retry %s for %s", count, command); - retryCountMap.put(command, count + 1); - // Wait between retries - authenticationResponseCache.invalidateAll(); - Uninterruptibles.sleepUninterruptibly(5, TimeUnit.SECONDS); - retry = true; - } + // Retry just in case + logger.debug("invalidating authentication token - retry %s for %s", count, command); + retryCountMap.put(command, count + 1); + // Wait between retries + authenticationResponseCache.invalidateAll(); + Uninterruptibles.sleepUninterruptibly(5, TimeUnit.SECONDS); + retry = true; } } - break; - } - return retry; - - } finally { - releasePayload(response); + } + break; } + return retry; } } diff --git a/common/openstack/src/main/java/org/jclouds/openstack/keystone/v1_1/handlers/RetryOnRenew.java b/common/openstack/src/main/java/org/jclouds/openstack/keystone/v1_1/handlers/RetryOnRenew.java index 47eb259701..e03926f3d2 100644 --- a/common/openstack/src/main/java/org/jclouds/openstack/keystone/v1_1/handlers/RetryOnRenew.java +++ b/common/openstack/src/main/java/org/jclouds/openstack/keystone/v1_1/handlers/RetryOnRenew.java @@ -16,9 +16,6 @@ */ package org.jclouds.openstack.keystone.v1_1.handlers; -import static org.jclouds.http.HttpUtils.closeClientButKeepContentStream; -import static org.jclouds.http.HttpUtils.releasePayload; - import java.util.concurrent.TimeUnit; import javax.annotation.Resource; @@ -80,51 +77,45 @@ public class RetryOnRenew implements HttpRetryHandler { @Override public boolean shouldRetryRequest(HttpCommand command, HttpResponse response) { boolean retry = false; // default - try { - switch (response.getStatusCode()) { - case 401: - // Do not retry on 401 from authentication request - Multimap headers = command.getCurrentRequest().getHeaders(); - if (headers != null && headers.containsKey(AuthHeaders.AUTH_USER) - && headers.containsKey(AuthHeaders.AUTH_KEY) && !headers.containsKey(AuthHeaders.AUTH_TOKEN)) { - retry = false; - } else { - closeClientButKeepContentStream(response); - // This is not an authentication request returning 401 - // Check if we already had seen this request - Integer count = retryCountMap.getIfPresent(command); + switch (response.getStatusCode()) { + case 401: + // Do not retry on 401 from authentication request + Multimap headers = command.getCurrentRequest().getHeaders(); + if (headers != null && headers.containsKey(AuthHeaders.AUTH_USER) + && headers.containsKey(AuthHeaders.AUTH_KEY) && !headers.containsKey(AuthHeaders.AUTH_TOKEN)) { + retry = false; + } else { + // This is not an authentication request returning 401 + // Check if we already had seen this request + Integer count = retryCountMap.getIfPresent(command); - if (count == null) { - // First time this non-authentication request failed - logger.debug("invalidating authentication token - first time for %s", command); - retryCountMap.put(command, 1); - authenticationResponseCache.invalidateAll(); - retry = true; + if (count == null) { + // First time this non-authentication request failed + logger.debug("invalidating authentication token - first time for %s", command); + retryCountMap.put(command, 1); + authenticationResponseCache.invalidateAll(); + 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); + retry = false; } else { - // This request has failed before - if (count + 1 >= NUM_RETRIES) { - logger.debug("too many 401s - giving up after: %s for %s", count, command); - retry = false; - } else { - // Retry just in case - logger.debug("invalidating authentication token - retry %s for %s", count, command); - retryCountMap.put(command, count + 1); - // Wait between retries - authenticationResponseCache.invalidateAll(); - Uninterruptibles.sleepUninterruptibly(5, TimeUnit.SECONDS); - retry = true; - } + // Retry just in case + logger.debug("invalidating authentication token - retry %s for %s", count, command); + retryCountMap.put(command, count + 1); + // Wait between retries + authenticationResponseCache.invalidateAll(); + Uninterruptibles.sleepUninterruptibly(5, TimeUnit.SECONDS); + retry = true; } } - break; - case 408: - return backoffHandler.shouldRetryRequest(command, response); - } - return retry; - - } finally { - releasePayload(response); + } + break; + case 408: + return backoffHandler.shouldRetryRequest(command, response); } + return retry; } } diff --git a/common/openstack/src/test/java/org/jclouds/openstack/keystone/v1_1/handlers/RetryOnRenewTest.java b/common/openstack/src/test/java/org/jclouds/openstack/keystone/v1_1/handlers/RetryOnRenewTest.java index ec3d88d739..8ee89f1b66 100644 --- a/common/openstack/src/test/java/org/jclouds/openstack/keystone/v1_1/handlers/RetryOnRenewTest.java +++ b/common/openstack/src/test/java/org/jclouds/openstack/keystone/v1_1/handlers/RetryOnRenewTest.java @@ -110,14 +110,11 @@ public class RetryOnRenewTest { @Test public void test408ShouldRetry() { HttpCommand command = createMock(HttpCommand.class); - HttpRequest request = createMock(HttpRequest.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(); @@ -145,7 +142,6 @@ public class RetryOnRenewTest { LoadingCache cache = createMock(LoadingCache.class); BackoffLimitedRetryHandler backoffHandler = createMock(BackoffLimitedRetryHandler.class); - expect(response.getPayload()).andReturn(Payloads.newStringPayload("")).times(2); expect(response.getStatusCode()).andReturn(404).once(); replay(command);