From 6d7c2ae94217bfc4e360676e083c255f899a9974 Mon Sep 17 00:00:00 2001 From: Nikita Konev Date: Sat, 3 Aug 2019 17:07:01 +0300 Subject: [PATCH] #821 read timeout is not retried. Thanks to @Aloren's https://github.com/spring-cloud/spring-cloud-gateway/pull/823 --- .../factory/RetryGatewayFilterFactory.java | 58 ++++++++++++++----- ...yGatewayFilterFactoryIntegrationTests.java | 34 ++++++++++- 2 files changed, 73 insertions(+), 19 deletions(-) diff --git a/spring-cloud-gateway-core/src/main/java/org/springframework/cloud/gateway/filter/factory/RetryGatewayFilterFactory.java b/spring-cloud-gateway-core/src/main/java/org/springframework/cloud/gateway/filter/factory/RetryGatewayFilterFactory.java index 7c49b5b6..00d85788 100644 --- a/spring-cloud-gateway-core/src/main/java/org/springframework/cloud/gateway/filter/factory/RetryGatewayFilterFactory.java +++ b/spring-cloud-gateway-core/src/main/java/org/springframework/cloud/gateway/filter/factory/RetryGatewayFilterFactory.java @@ -24,6 +24,7 @@ import java.util.Collections; import java.util.List; import java.util.Set; import java.util.function.Predicate; +import java.util.function.Supplier; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; @@ -93,16 +94,17 @@ public class RetryGatewayFilterFactory .anyMatch(series -> statusCode.series().equals(series)); } + final boolean finalRetryableStatusCode = retryableStatusCode; trace("retryableStatusCode: %b, statusCode %s, configured statuses %s, configured series %s", - retryableStatusCode, statusCode, retryConfig.getStatuses(), - retryConfig.getSeries()); + () -> finalRetryableStatusCode, () -> statusCode, + retryConfig::getStatuses, retryConfig::getSeries); HttpMethod httpMethod = exchange.getRequest().getMethod(); boolean retryableMethod = retryConfig.getMethods().contains(httpMethod); trace("retryableMethod: %b, httpMethod %s, configured methods %s", - retryableMethod, httpMethod, retryConfig.getMethods()); - return retryableMethod && retryableStatusCode; + () -> retryableMethod, () -> httpMethod, retryConfig::getMethods); + return retryableMethod && finalRetryableStatusCode; }; statusCodeRepeat = Repeat.onlyIf(repeatPredicate) @@ -123,17 +125,20 @@ public class RetryGatewayFilterFactory return false; } - for (Class clazz : retryConfig.getExceptions()) { - if (clazz.isInstance(context.exception())) { - trace("exception is retryable %s, configured exceptions", - context.exception().getClass().getName(), - retryConfig.getExceptions()); + Throwable exception = context.exception(); + for (Class retryableClass : retryConfig + .getExceptions()) { + if (retryableClass.isInstance(exception) || (exception != null + && retryableClass.isInstance(exception.getCause()))) { + trace("exception or its cause is retryable %s, configured exceptions %s", + () -> getExceptionNameWithCause(exception), + retryConfig::getExceptions); return true; } } - trace("exception is not retryable %s, configured exceptions", - context.exception().getClass().getName(), - retryConfig.getExceptions()); + trace("exception or its cause is not retryable %s, configured exceptions %s", + () -> getExceptionNameWithCause(exception), + retryConfig::getExceptions); return false; }; exceptionRetry = Retry.onlyIf(retryContextPredicate) @@ -166,6 +171,20 @@ public class RetryGatewayFilterFactory }; } + private String getExceptionNameWithCause(Throwable exception) { + if (exception != null) { + StringBuilder builder = new StringBuilder(exception.getClass().getName()); + Throwable cause = exception.getCause(); + if (cause != null) { + builder.append("{cause=").append(cause.getClass().getName()).append("}"); + } + return builder.toString(); + } + else { + return "null"; + } + } + private Backoff getBackoff(BackoffConfig backoff) { return Backoff.exponential(backoff.firstBackoff, backoff.maxBackoff, backoff.factor, backoff.basedOnPreviousValue); @@ -177,8 +196,8 @@ public class RetryGatewayFilterFactory // TODO: deal with null iteration boolean exceeds = iteration != null && iteration >= retryConfig.getRetries(); - trace("exceedsMaxIterations %b, iteration %d, configured retries %d", exceeds, - iteration, retryConfig.getRetries()); + trace("exceedsMaxIterations %b, iteration %d, configured retries %d", + () -> exceeds, () -> iteration, retryConfig::getRetries); return exceeds; } @@ -213,7 +232,7 @@ public class RetryGatewayFilterFactory int iteration = exchange .getAttributeOrDefault(RETRY_ITERATION_KEY, -1); int newIteration = iteration + 1; - trace("setting new iteration in attr %d", newIteration); + trace("setting new iteration in attr %d", () -> newIteration); exchange.getAttributes().put(RETRY_ITERATION_KEY, newIteration); }); @@ -234,8 +253,15 @@ public class RetryGatewayFilterFactory }; } - private void trace(String message, Object... args) { + @SafeVarargs + private final void trace(String message, Supplier... argSuppliers) { if (log.isTraceEnabled()) { + Object[] args = new Object[argSuppliers.length]; + int i = 0; + for (Supplier a : argSuppliers) { + args[i] = a.get(); + ++i; + } log.trace(String.format(message, args)); } } diff --git a/spring-cloud-gateway-core/src/test/java/org/springframework/cloud/gateway/filter/factory/RetryGatewayFilterFactoryIntegrationTests.java b/spring-cloud-gateway-core/src/test/java/org/springframework/cloud/gateway/filter/factory/RetryGatewayFilterFactoryIntegrationTests.java index 39a8fe14..aa084f2f 100644 --- a/spring-cloud-gateway-core/src/test/java/org/springframework/cloud/gateway/filter/factory/RetryGatewayFilterFactoryIntegrationTests.java +++ b/spring-cloud-gateway-core/src/test/java/org/springframework/cloud/gateway/filter/factory/RetryGatewayFilterFactoryIntegrationTests.java @@ -59,7 +59,9 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.springframework.boot.test.context.SpringBootTest.WebEnvironment.RANDOM_PORT; @RunWith(SpringRunner.class) -@SpringBootTest(webEnvironment = RANDOM_PORT) +@SpringBootTest(webEnvironment = RANDOM_PORT, properties = { + "spring.cloud.gateway.httpclient.connect-timeout=500", + "spring.cloud.gateway.httpclient.response-timeout=2s" }) @DirtiesContext public class RetryGatewayFilterFactoryIntegrationTests extends BaseWebClientTests { @@ -113,6 +115,16 @@ public class RetryGatewayFilterFactoryIntegrationTests extends BaseWebClientTest .exchange().expectStatus().isOk().expectBody(String.class).isEqualTo("3"); } + @Test + public void retriesSleepyRequest() throws Exception { + testClient.mutate().responseTimeout(Duration.ofSeconds(10)).build().get() + .uri("/sleep?key=sleepyRequest&millis=3000") + .header(HttpHeaders.HOST, "www.retryjava.org").exchange().expectStatus() + .isEqualTo(HttpStatus.GATEWAY_TIMEOUT); + + assertThat(TestConfig.map.get("sleepyRequest")).isNotNull().hasValue(3); + } + @Test @SuppressWarnings("unchecked") public void retryFilterLoadBalancedWithMultipleServers() { @@ -148,11 +160,27 @@ public class RetryGatewayFilterFactoryIntegrationTests extends BaseWebClientTest Log log = LogFactory.getLog(getClass()); - ConcurrentHashMap map = new ConcurrentHashMap<>(); + static ConcurrentHashMap map = new ConcurrentHashMap<>(); @Value("${test.uri}") private String uri; + @RequestMapping("/httpbin/sleep") + public ResponseEntity sleep(@RequestParam("key") String key, + @RequestParam("millis") long millisToSleep) { + AtomicInteger num = getCount(key); + int retryCount = num.incrementAndGet(); + log.warn("Retry count: " + retryCount); + try { + Thread.sleep(millisToSleep); + } + catch (InterruptedException e) { + } + return ResponseEntity.status(HttpStatus.OK) + .header("X-Retry-Count", String.valueOf(retryCount)) + .body("slept " + millisToSleep + " ms"); + } + @RequestMapping("/httpbin/retryalwaysfail") public ResponseEntity retryalwaysfail(@RequestParam("key") String key, @RequestParam(name = "count", defaultValue = "3") int count) { @@ -194,7 +222,7 @@ public class RetryGatewayFilterFactoryIntegrationTests extends BaseWebClientTest .body(body); } - AtomicInteger getCount(@RequestParam("key") String key) { + AtomicInteger getCount(String key) { return map.computeIfAbsent(key, s -> new AtomicInteger()); }