From 027670277031cb8e446e7ab0aea1589ab8e6e428 Mon Sep 17 00:00:00 2001 From: Michele Mancioppi Date: Wed, 17 Oct 2018 23:09:33 +0200 Subject: [PATCH] Treat corner cases of Hystrix exception handling * Treat HttpClient timeouts the same as Hystrix timeouts * Propagate ResponseStatusException and throwables with @ResponseStatus when cause for COMMAND_EXECUTION failure Fix #554 --- .../factory/HystrixGatewayFilterFactory.java | 26 ++++++++++++++++--- .../gateway/support/TimeoutException.java | 5 ++++ .../NettyRoutingFilterIntegrationTests.java | 5 ++-- .../HystrixGatewayFilterFactoryTests.java | 23 +++++++++++++++- 4 files changed, 52 insertions(+), 7 deletions(-) diff --git a/spring-cloud-gateway-core/src/main/java/org/springframework/cloud/gateway/filter/factory/HystrixGatewayFilterFactory.java b/spring-cloud-gateway-core/src/main/java/org/springframework/cloud/gateway/filter/factory/HystrixGatewayFilterFactory.java index b0b7b71a..90d22906 100644 --- a/spring-cloud-gateway-core/src/main/java/org/springframework/cloud/gateway/filter/factory/HystrixGatewayFilterFactory.java +++ b/spring-cloud-gateway-core/src/main/java/org/springframework/cloud/gateway/filter/factory/HystrixGatewayFilterFactory.java @@ -36,22 +36,24 @@ import rx.Subscription; import org.springframework.beans.factory.ObjectProvider; import org.springframework.cloud.gateway.filter.GatewayFilter; import org.springframework.cloud.gateway.filter.GatewayFilterChain; -import org.springframework.http.HttpStatus; +import org.springframework.cloud.gateway.support.TimeoutException; +import org.springframework.core.annotation.AnnotatedElementUtils; import org.springframework.http.server.reactive.ServerHttpRequest; import org.springframework.util.Assert; import org.springframework.util.StringUtils; +import org.springframework.web.bind.annotation.ResponseStatus; import org.springframework.web.reactive.DispatcherHandler; import org.springframework.web.server.ResponseStatusException; import org.springframework.web.server.ServerWebExchange; import org.springframework.web.util.UriComponentsBuilder; -import static com.netflix.hystrix.exception.HystrixRuntimeException.FailureType.TIMEOUT; import static org.springframework.cloud.gateway.support.ServerWebExchangeUtils.GATEWAY_REQUEST_URL_ATTR; import static org.springframework.cloud.gateway.support.ServerWebExchangeUtils.containsEncodedParts; /** * Depends on `spring-cloud-starter-netflix-hystrix`, {@see http://cloud.spring.io/spring-cloud-netflix/} * @author Spencer Gibb + * @author Michele Mancioppi */ public class HystrixGatewayFilterFactory extends AbstractGatewayFilterFactory { @@ -101,8 +103,24 @@ public class HystrixGatewayFilterFactory extends AbstractGatewayFilterFactory>) throwable -> { if (throwable instanceof HystrixRuntimeException) { HystrixRuntimeException e = (HystrixRuntimeException) throwable; - if (e.getFailureType() == TIMEOUT) { - return Mono.error(new ResponseStatusException(HttpStatus.GATEWAY_TIMEOUT)); + HystrixRuntimeException.FailureType failureType = e.getFailureType(); + + switch (failureType) { + case TIMEOUT: + return Mono.error(new TimeoutException()); + case COMMAND_EXCEPTION: { + Throwable cause = e.getCause(); + + /* + * We forsake here the null check for cause as HystrixRuntimeException will + * always have a cause if the failure type is COMMAND_EXCEPTION. + */ + if (cause instanceof ResponseStatusException || AnnotatedElementUtils + .findMergedAnnotation(cause.getClass(), ResponseStatus.class) != null) { + return Mono.error(cause); + } + } + default: break; } } return Mono.error(throwable); diff --git a/spring-cloud-gateway-core/src/main/java/org/springframework/cloud/gateway/support/TimeoutException.java b/spring-cloud-gateway-core/src/main/java/org/springframework/cloud/gateway/support/TimeoutException.java index 8d429bc2..2a8e518c 100644 --- a/spring-cloud-gateway-core/src/main/java/org/springframework/cloud/gateway/support/TimeoutException.java +++ b/spring-cloud-gateway-core/src/main/java/org/springframework/cloud/gateway/support/TimeoutException.java @@ -17,6 +17,11 @@ package org.springframework.cloud.gateway.support; +import static org.springframework.http.HttpStatus.GATEWAY_TIMEOUT; + +import org.springframework.web.bind.annotation.ResponseStatus; + +@ResponseStatus(value = GATEWAY_TIMEOUT, reason = "Response took longer than configured timeout") public class TimeoutException extends Exception { public TimeoutException() { diff --git a/spring-cloud-gateway-core/src/test/java/org/springframework/cloud/gateway/filter/NettyRoutingFilterIntegrationTests.java b/spring-cloud-gateway-core/src/test/java/org/springframework/cloud/gateway/filter/NettyRoutingFilterIntegrationTests.java index aefd24ae..4148a7ba 100644 --- a/spring-cloud-gateway-core/src/test/java/org/springframework/cloud/gateway/filter/NettyRoutingFilterIntegrationTests.java +++ b/spring-cloud-gateway-core/src/test/java/org/springframework/cloud/gateway/filter/NettyRoutingFilterIntegrationTests.java @@ -27,6 +27,7 @@ import org.springframework.boot.autoconfigure.EnableAutoConfiguration; import org.springframework.boot.test.context.SpringBootTest; import org.springframework.cloud.gateway.test.BaseWebClientTests; import org.springframework.context.annotation.Import; +import org.springframework.http.HttpStatus; import org.springframework.test.annotation.DirtiesContext; import org.springframework.test.context.junit4.SpringRunner; @@ -44,11 +45,11 @@ public class NettyRoutingFilterIntegrationTests extends BaseWebClientTests { testClient.get() .uri("/delay/5") .exchange() - .expectStatus().is5xxServerError() + .expectStatus().isEqualTo(HttpStatus.GATEWAY_TIMEOUT) .expectBody(Map.class) .consumeWith(result -> { Map body = result.getResponseBody(); - assertThat(body).containsEntry("message", "Response took longer than timeout: PT3S"); + assertThat(body).containsEntry("message", "Response took longer than configured timeout"); }); } diff --git a/spring-cloud-gateway-core/src/test/java/org/springframework/cloud/gateway/filter/factory/HystrixGatewayFilterFactoryTests.java b/spring-cloud-gateway-core/src/test/java/org/springframework/cloud/gateway/filter/factory/HystrixGatewayFilterFactoryTests.java index 54773770..a47faecf 100644 --- a/spring-cloud-gateway-core/src/test/java/org/springframework/cloud/gateway/filter/factory/HystrixGatewayFilterFactoryTests.java +++ b/spring-cloud-gateway-core/src/test/java/org/springframework/cloud/gateway/filter/factory/HystrixGatewayFilterFactoryTests.java @@ -74,6 +74,20 @@ public class HystrixGatewayFilterFactoryTests extends BaseWebClientTests { .isEqualTo(String.valueOf(HttpStatus.GATEWAY_TIMEOUT.value())); } + /* + * Tests that timeouts bubbling from the underpinning WebClient are treated the same as + * Hystrix timeouts in terms of outside response. (Internally, timeouts from the WebClient + * are seen as command failures and trigger the opening of circuit breakers the same way + * timeouts do; it may be confusing in terms of the Hystrix metrics though) + */ + @Test + public void hystrixTimeoutFromWebClient() { + testClient.get().uri("/delay/10") + .header("Host", "www.hystrixresponsestall.org") + .exchange() + .expectStatus().isEqualTo(HttpStatus.GATEWAY_TIMEOUT); + } + @Test public void hystrixFilterFallback() { testClient.get().uri("/delay/3?a=b") @@ -157,11 +171,18 @@ public class HystrixGatewayFilterFactoryTests extends BaseWebClientTests { .filters(f -> f.prefixPath("/httpbin") .hystrix(config -> {})) .uri("lb:badservice")) + /* + * This is a route encapsulated in a hystrix command that is ready to wait + * for a response far longer than the underpinning WebClient would. + */ + .route("hystrix_response_stall", r -> r.host("**.hystrixresponsestall.org") + .filters(f -> f.prefixPath("/httpbin") + .hystrix(config -> config.setName("stalling-command"))) + .uri(uri)) .build(); } } - protected static class TestBadRibbonConfig { @LocalServerPort