From 08d037b4b4c91774304675ff7f4beca83f7a4a0a Mon Sep 17 00:00:00 2001 From: Arkin Yetis Date: Thu, 7 Jun 2018 19:26:49 -0700 Subject: [PATCH] Disables repeat/retry mechanisms depending on config. Changes retry filter to allow empty status and series config when exceptions is not empty. Fixes gh-352. --- .../factory/RetryGatewayFilterFactory.java | 116 ++++++++++-------- .../route/builder/GatewayFilterSpec.java | 5 - 2 files changed, 66 insertions(+), 55 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 634f4746..47a6f735 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 @@ -22,10 +22,10 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.List; import java.util.function.Predicate; -import java.util.logging.Level; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; +import org.reactivestreams.Publisher; import reactor.core.publisher.Mono; import reactor.retry.Repeat; import reactor.retry.RepeatContext; @@ -53,49 +53,56 @@ public class RetryGatewayFilterFactory extends AbstractGatewayFilterFactory> repeatPredicate = context -> { - ServerWebExchange exchange = context.applicationContext(); - if (exceedsMaxIterations(exchange, retryConfig)) { - return false; - } + Repeat statusCodeRepeat = null; + if (!retryConfig.getStatuses().isEmpty() || !retryConfig.getSeries().isEmpty()) { + Predicate> repeatPredicate = context -> { + ServerWebExchange exchange = context.applicationContext(); + if (exceedsMaxIterations(exchange, retryConfig)) { + return false; + } - HttpStatus statusCode = exchange.getResponse().getStatusCode(); - HttpMethod httpMethod = exchange.getRequest().getMethod(); + HttpStatus statusCode = exchange.getResponse().getStatusCode(); + HttpMethod httpMethod = exchange.getRequest().getMethod(); - boolean retryableStatusCode = retryConfig.getStatuses().contains(statusCode); + boolean retryableStatusCode = retryConfig.getStatuses().contains(statusCode); - if (!retryableStatusCode && statusCode != null) { // null status code might mean a network exception? - // try the series - retryableStatusCode = retryConfig.getSeries().stream() - .anyMatch(series -> statusCode.series().equals(series)); - } + if (!retryableStatusCode && statusCode != null) { // null status code might mean a network exception? + // try the series + retryableStatusCode = retryConfig.getSeries().stream() + .anyMatch(series -> statusCode.series().equals(series)); + } - boolean retryableMethod = retryConfig.getMethods().contains(httpMethod); - return retryableMethod && retryableStatusCode; - }; + boolean retryableMethod = retryConfig.getMethods().contains(httpMethod); + return retryableMethod && retryableStatusCode; + }; - Repeat repeat = Repeat.onlyIf(repeatPredicate) - .doOnRepeat(context -> reset(context.applicationContext())); + statusCodeRepeat = Repeat.onlyIf(repeatPredicate) + .doOnRepeat(context -> reset(context.applicationContext())); + } //TODO: support timeout, backoff, jitter, etc... in Builder - Predicate> retryContextPredicate = context -> { - if (exceedsMaxIterations(context.applicationContext(), retryConfig)) { - return false; - } - - for (Class clazz : retryConfig.getExceptions()) { - if (clazz.isInstance(context.exception())) { - return true; + Retry exceptionRetry = null; + if (!retryConfig.getExceptions().isEmpty()) { + Predicate> retryContextPredicate = context -> { + if (exceedsMaxIterations(context.applicationContext(), retryConfig)) { + return false; } - } - return false; - }; - Retry reactorRetry = Retry.onlyIf(retryContextPredicate) - .doOnRetry(context -> reset(context.applicationContext())) - .retryMax(retryConfig.getRetries()); - return apply(repeat, reactorRetry); + for (Class clazz : retryConfig.getExceptions()) { + if (clazz.isInstance(context.exception())) { + return true; + } + } + return false; + }; + exceptionRetry = Retry.onlyIf(retryContextPredicate) + .doOnRetry(context -> reset(context.applicationContext())) + .retryMax(retryConfig.getRetries()); + } + + + return apply(statusCodeRepeat, exceptionRetry); } public boolean exceedsMaxIterations(ServerWebExchange exchange, RetryConfig retryConfig) { @@ -110,23 +117,32 @@ public class RetryGatewayFilterFactory extends AbstractGatewayFilterFactory repeat) { - return apply(repeat, Retry.onlyIf(ctxt -> false)); - } - public GatewayFilter apply(Repeat repeat, Retry retry) { return (exchange, chain) -> { - log.trace("Entering retry-filter"); + if (log.isTraceEnabled()) { + log.trace("Entering retry-filter"); + } - return Mono.fromDirect(chain.filter(exchange) - .doOnSuccessOrError((aVoid, throwable) -> { - int iteration = exchange.getAttributeOrDefault(RETRY_ITERATION_KEY, -1); - exchange.getAttributes().put(RETRY_ITERATION_KEY, iteration + 1); - }) - // .log("retry-filter", Level.INFO) - .retryWhen(retry.withApplicationContext(exchange)) - .repeatWhen(repeat.withApplicationContext(exchange))); + // chain.filter returns a Mono + Publisher publisher = chain.filter(exchange) + //.log("retry-filter", Level.INFO) + .doOnSuccessOrError((aVoid, throwable) -> { + int iteration = exchange.getAttributeOrDefault(RETRY_ITERATION_KEY, -1); + exchange.getAttributes().put(RETRY_ITERATION_KEY, iteration + 1); + }); + + if (retry != null) { + // retryWhen returns a Mono + // retry needs to go before repeat + publisher = ((Mono)publisher).retryWhen(retry.withApplicationContext(exchange)); + } + if (repeat != null) { + // repeatWhen returns a Flux + // so this needs to be last and the variable a Publisher + publisher = ((Mono)publisher).repeatWhen(repeat.withApplicationContext(exchange)); + } + + return Mono.fromDirect(publisher); }; } @@ -178,8 +194,8 @@ public class RetryGatewayFilterFactory extends AbstractGatewayFilterFactory 0, "retries must be greater than 0"); - Assert.isTrue(!this.series.isEmpty() || !this.statuses.isEmpty(), - "series and status may not both be empty"); + Assert.isTrue(!this.series.isEmpty() || !this.statuses.isEmpty() || !this.exceptions.isEmpty(), + "series, status and exceptions may not all be empty"); Assert.notEmpty(this.methods, "methods may not be empty"); } diff --git a/spring-cloud-gateway-core/src/main/java/org/springframework/cloud/gateway/route/builder/GatewayFilterSpec.java b/spring-cloud-gateway-core/src/main/java/org/springframework/cloud/gateway/route/builder/GatewayFilterSpec.java index 0fa49a4b..4a0722fd 100644 --- a/spring-cloud-gateway-core/src/main/java/org/springframework/cloud/gateway/route/builder/GatewayFilterSpec.java +++ b/spring-cloud-gateway-core/src/main/java/org/springframework/cloud/gateway/route/builder/GatewayFilterSpec.java @@ -234,11 +234,6 @@ public class GatewayFilterSpec extends UriSpec { return filter(getBean(RetryGatewayFilterFactory.class).apply(repeat, retry)); } - @Deprecated - public GatewayFilterSpec retry(Repeat repeat) { - return filter(getBean(RetryGatewayFilterFactory.class).apply(repeat)); - } - @SuppressWarnings("unchecked") public GatewayFilterSpec secureHeaders() { return filter(getBean(SecureHeadersGatewayFilterFactory.class).apply(c -> {}));