From dd518a812b0e90f976cfc02258939ad99c251bcf Mon Sep 17 00:00:00 2001 From: Olga Maciaszek-Sharma Date: Thu, 1 Oct 2020 13:20:24 -0500 Subject: [PATCH] Add support for load-balanced retries. (#408) --- ...DefaultFeignLoadBalancerConfiguration.java | 23 ++ ...pClientFeignLoadBalancerConfiguration.java | 21 ++ .../OkHttpFeignLoadBalancerConfiguration.java | 21 ++ .../OnRetryNotEnabledCondition.java | 56 +++++ ...ryableBlockingFeignLoadBalancerClient.java | 185 ++++++++++++++++ .../openfeign/ribbon/FeignLoadBalancer.java | 5 +- .../ribbon/RetryableFeignLoadBalancer.java | 59 +++--- ...ignLoadBalancerAutoConfigurationTests.java | 53 ++++- ...eBlockingFeignLoadBalancerClientTests.java | 197 ++++++++++++++++++ 9 files changed, 580 insertions(+), 40 deletions(-) create mode 100644 spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/loadbalancer/OnRetryNotEnabledCondition.java create mode 100644 spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/loadbalancer/RetryableBlockingFeignLoadBalancerClient.java create mode 100644 spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/loadbalancer/RetryableBlockingFeignLoadBalancerClientTests.java diff --git a/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/loadbalancer/DefaultFeignLoadBalancerConfiguration.java b/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/loadbalancer/DefaultFeignLoadBalancerConfiguration.java index e549c940..950da501 100644 --- a/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/loadbalancer/DefaultFeignLoadBalancerConfiguration.java +++ b/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/loadbalancer/DefaultFeignLoadBalancerConfiguration.java @@ -16,12 +16,20 @@ package org.springframework.cloud.openfeign.loadbalancer; +import java.util.List; + import feign.Client; +import org.springframework.boot.autoconfigure.condition.ConditionalOnBean; +import org.springframework.boot.autoconfigure.condition.ConditionalOnClass; import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean; +import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty; +import org.springframework.cloud.client.loadbalancer.LoadBalancedRetryFactory; import org.springframework.cloud.loadbalancer.blocking.client.BlockingLoadBalancerClient; import org.springframework.context.annotation.Bean; +import org.springframework.context.annotation.Conditional; import org.springframework.context.annotation.Configuration; +import org.springframework.core.annotation.AnnotationAwareOrderComparator; /** * Configuration instantiating a {@link BlockingLoadBalancerClient}-based {@link Client} @@ -35,9 +43,24 @@ class DefaultFeignLoadBalancerConfiguration { @Bean @ConditionalOnMissingBean + @Conditional(OnRetryNotEnabledCondition.class) public Client feignClient(BlockingLoadBalancerClient loadBalancerClient) { return new FeignBlockingLoadBalancerClient(new Client.Default(null, null), loadBalancerClient); } + @Bean + @ConditionalOnMissingBean + @ConditionalOnClass(name = "org.springframework.retry.support.RetryTemplate") + @ConditionalOnBean(LoadBalancedRetryFactory.class) + @ConditionalOnProperty(value = "spring.cloud.loadbalancer.retry.enabled", + havingValue = "true", matchIfMissing = true) + public Client feignRetryClient(BlockingLoadBalancerClient loadBalancerClient, + List loadBalancedRetryFactories) { + AnnotationAwareOrderComparator.sort(loadBalancedRetryFactories); + return new RetryableBlockingFeignLoadBalancerClient( + new Client.Default(null, null), loadBalancerClient, + loadBalancedRetryFactories.get(0)); + } + } diff --git a/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/loadbalancer/HttpClientFeignLoadBalancerConfiguration.java b/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/loadbalancer/HttpClientFeignLoadBalancerConfiguration.java index 171fb172..75483e61 100644 --- a/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/loadbalancer/HttpClientFeignLoadBalancerConfiguration.java +++ b/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/loadbalancer/HttpClientFeignLoadBalancerConfiguration.java @@ -16,6 +16,8 @@ package org.springframework.cloud.openfeign.loadbalancer; +import java.util.List; + import feign.Client; import feign.httpclient.ApacheHttpClient; import org.apache.http.client.HttpClient; @@ -24,11 +26,14 @@ import org.springframework.boot.autoconfigure.condition.ConditionalOnBean; import org.springframework.boot.autoconfigure.condition.ConditionalOnClass; import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean; import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty; +import org.springframework.cloud.client.loadbalancer.LoadBalancedRetryFactory; import org.springframework.cloud.loadbalancer.blocking.client.BlockingLoadBalancerClient; import org.springframework.cloud.openfeign.clientconfig.HttpClientFeignConfiguration; import org.springframework.context.annotation.Bean; +import org.springframework.context.annotation.Conditional; import org.springframework.context.annotation.Configuration; import org.springframework.context.annotation.Import; +import org.springframework.core.annotation.AnnotationAwareOrderComparator; /** * Configuration instantiating a {@link BlockingLoadBalancerClient}-based {@link Client} @@ -46,10 +51,26 @@ class HttpClientFeignLoadBalancerConfiguration { @Bean @ConditionalOnMissingBean + @Conditional(OnRetryNotEnabledCondition.class) public Client feignClient(BlockingLoadBalancerClient loadBalancerClient, HttpClient httpClient) { ApacheHttpClient delegate = new ApacheHttpClient(httpClient); return new FeignBlockingLoadBalancerClient(delegate, loadBalancerClient); } + @Bean + @ConditionalOnMissingBean + @ConditionalOnClass(name = "org.springframework.retry.support.RetryTemplate") + @ConditionalOnBean(LoadBalancedRetryFactory.class) + @ConditionalOnProperty(value = "spring.cloud.loadbalancer.retry.enabled", + havingValue = "true", matchIfMissing = true) + public Client feignRetryClient(BlockingLoadBalancerClient loadBalancerClient, + HttpClient httpClient, + List loadBalancedRetryFactories) { + AnnotationAwareOrderComparator.sort(loadBalancedRetryFactories); + ApacheHttpClient delegate = new ApacheHttpClient(httpClient); + return new RetryableBlockingFeignLoadBalancerClient(delegate, loadBalancerClient, + loadBalancedRetryFactories.get(0)); + } + } diff --git a/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/loadbalancer/OkHttpFeignLoadBalancerConfiguration.java b/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/loadbalancer/OkHttpFeignLoadBalancerConfiguration.java index 36f7adab..0179d35f 100644 --- a/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/loadbalancer/OkHttpFeignLoadBalancerConfiguration.java +++ b/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/loadbalancer/OkHttpFeignLoadBalancerConfiguration.java @@ -16,6 +16,8 @@ package org.springframework.cloud.openfeign.loadbalancer; +import java.util.List; + import feign.Client; import feign.okhttp.OkHttpClient; @@ -23,11 +25,14 @@ import org.springframework.boot.autoconfigure.condition.ConditionalOnBean; import org.springframework.boot.autoconfigure.condition.ConditionalOnClass; import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean; import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty; +import org.springframework.cloud.client.loadbalancer.LoadBalancedRetryFactory; import org.springframework.cloud.loadbalancer.blocking.client.BlockingLoadBalancerClient; import org.springframework.cloud.openfeign.clientconfig.OkHttpFeignConfiguration; import org.springframework.context.annotation.Bean; +import org.springframework.context.annotation.Conditional; import org.springframework.context.annotation.Configuration; import org.springframework.context.annotation.Import; +import org.springframework.core.annotation.AnnotationAwareOrderComparator; /** * Configuration instantiating a {@link BlockingLoadBalancerClient}-based {@link Client} @@ -45,10 +50,26 @@ class OkHttpFeignLoadBalancerConfiguration { @Bean @ConditionalOnMissingBean + @Conditional(OnRetryNotEnabledCondition.class) public Client feignClient(okhttp3.OkHttpClient okHttpClient, BlockingLoadBalancerClient loadBalancerClient) { OkHttpClient delegate = new OkHttpClient(okHttpClient); return new FeignBlockingLoadBalancerClient(delegate, loadBalancerClient); } + @Bean + @ConditionalOnMissingBean + @ConditionalOnClass(name = "org.springframework.retry.support.RetryTemplate") + @ConditionalOnBean(LoadBalancedRetryFactory.class) + @ConditionalOnProperty(value = "spring.cloud.loadbalancer.retry.enabled", + havingValue = "true", matchIfMissing = true) + public Client feignRetryClient(BlockingLoadBalancerClient loadBalancerClient, + okhttp3.OkHttpClient okHttpClient, + List loadBalancedRetryFactories) { + AnnotationAwareOrderComparator.sort(loadBalancedRetryFactories); + OkHttpClient delegate = new OkHttpClient(okHttpClient); + return new RetryableBlockingFeignLoadBalancerClient(delegate, loadBalancerClient, + loadBalancedRetryFactories.get(0)); + } + } diff --git a/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/loadbalancer/OnRetryNotEnabledCondition.java b/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/loadbalancer/OnRetryNotEnabledCondition.java new file mode 100644 index 00000000..cef7c5c7 --- /dev/null +++ b/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/loadbalancer/OnRetryNotEnabledCondition.java @@ -0,0 +1,56 @@ +/* + * Copyright 2013-2020 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.cloud.openfeign.loadbalancer; + +import org.springframework.boot.autoconfigure.condition.AnyNestedCondition; +import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean; +import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingClass; +import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty; +import org.springframework.cloud.client.loadbalancer.LoadBalancedRetryFactory; +import org.springframework.retry.support.RetryTemplate; + +/** + * A condition that verifies that {@link RetryTemplate} is on the classpath, a + * {@link LoadBalancedRetryFactory} bean is present and + * spring.cloud.loadbalancer.retry.enabled is not set to false. + * + * @author Olga Maciaszek-Sharma + * @since 2.2.6 + */ +public class OnRetryNotEnabledCondition extends AnyNestedCondition { + + public OnRetryNotEnabledCondition() { + super(ConfigurationPhase.REGISTER_BEAN); + } + + @ConditionalOnMissingClass("org.springframework.retry.support.RetryTemplate") + static class OnNoRetryTemplateCondition { + + } + + @ConditionalOnMissingBean(LoadBalancedRetryFactory.class) + static class OnRetryFactoryCondition { + + } + + @ConditionalOnProperty(value = "spring.cloud.loadbalancer.retry.enabled", + havingValue = "false") + static class OnLoadBalancerRetryEnabledCondition { + + } + +} diff --git a/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/loadbalancer/RetryableBlockingFeignLoadBalancerClient.java b/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/loadbalancer/RetryableBlockingFeignLoadBalancerClient.java new file mode 100644 index 00000000..3931311d --- /dev/null +++ b/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/loadbalancer/RetryableBlockingFeignLoadBalancerClient.java @@ -0,0 +1,185 @@ +/* + * Copyright 2013-2020 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.cloud.openfeign.loadbalancer; + +import java.io.IOException; +import java.net.URI; +import java.util.ArrayList; +import java.util.Collection; +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +import feign.Client; +import feign.Request; +import feign.Response; +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; + +import org.springframework.cloud.client.ServiceInstance; +import org.springframework.cloud.client.loadbalancer.InterceptorRetryPolicy; +import org.springframework.cloud.client.loadbalancer.LoadBalancedRecoveryCallback; +import org.springframework.cloud.client.loadbalancer.LoadBalancedRetryContext; +import org.springframework.cloud.client.loadbalancer.LoadBalancedRetryFactory; +import org.springframework.cloud.client.loadbalancer.LoadBalancedRetryPolicy; +import org.springframework.cloud.client.loadbalancer.RetryableStatusCodeException; +import org.springframework.cloud.loadbalancer.blocking.client.BlockingLoadBalancerClient; +import org.springframework.http.HttpHeaders; +import org.springframework.http.HttpMethod; +import org.springframework.http.HttpRequest; +import org.springframework.retry.RetryListener; +import org.springframework.retry.backoff.BackOffPolicy; +import org.springframework.retry.backoff.NoBackOffPolicy; +import org.springframework.retry.policy.NeverRetryPolicy; +import org.springframework.retry.support.RetryTemplate; + +/** + * A {@link Client} implementation that provides Spring Retry support for requests + * load-balanced with Spring Cloud LoadBalancer. + * + * @author Olga Maciaszek-Sharma + * @since 2.2.6 + */ +public class RetryableBlockingFeignLoadBalancerClient implements Client { + + private static final Log LOG = LogFactory + .getLog(FeignBlockingLoadBalancerClient.class); + + private final Client delegate; + + private final BlockingLoadBalancerClient loadBalancerClient; + + private final LoadBalancedRetryFactory loadBalancedRetryFactory; + + public RetryableBlockingFeignLoadBalancerClient(Client delegate, + BlockingLoadBalancerClient loadBalancerClient, + LoadBalancedRetryFactory loadBalancedRetryFactory) { + this.delegate = delegate; + this.loadBalancerClient = loadBalancerClient; + this.loadBalancedRetryFactory = loadBalancedRetryFactory; + } + + @Override + public Response execute(Request request, Request.Options options) throws IOException { + final URI originalUri = URI.create(request.url()); + String serviceId = originalUri.getHost(); + final LoadBalancedRetryPolicy retryPolicy = loadBalancedRetryFactory + .createRetryPolicy(serviceId, loadBalancerClient); + RetryTemplate retryTemplate = buildRetryTemplate(serviceId, request, retryPolicy); + return retryTemplate.execute(context -> { + Request feignRequest = null; + // On retries the policy will choose the server and set it in the context + // and extract the server and update the request being made + if (context instanceof LoadBalancedRetryContext) { + ServiceInstance serviceInstance = ((LoadBalancedRetryContext) context) + .getServiceInstance(); + if (serviceInstance != null) { + if (LOG.isDebugEnabled()) { + LOG.debug(String.format( + "Using service instance from LoadBalancedRetryContext: %s", + serviceInstance)); + } + String reconstructedUrl = loadBalancerClient + .reconstructURI(serviceInstance, originalUri).toString(); + feignRequest = Request.create(request.httpMethod(), reconstructedUrl, + request.headers(), request.body(), request.charset(), + request.requestTemplate()); + } + } + if (feignRequest == null) { + if (LOG.isWarnEnabled()) { + LOG.warn( + "Service instance was not resolved, executing the original request"); + } + feignRequest = request; + } + Response response = delegate.execute(feignRequest, options); + int responseStatus = response.status(); + if (retryPolicy != null && retryPolicy.retryableStatusCode(responseStatus)) { + if (LOG.isDebugEnabled()) { + LOG.debug( + String.format("Retrying on status code: %d", responseStatus)); + } + response.close(); + throw new RetryableStatusCodeException(serviceId, responseStatus, + response, URI.create(request.url())); + } + return response; + }, new LoadBalancedRecoveryCallback() { + @Override + protected Response createResponse(Response response, URI uri) { + return response; + } + }); + } + + private RetryTemplate buildRetryTemplate(String serviceId, Request request, + LoadBalancedRetryPolicy retryPolicy) { + RetryTemplate retryTemplate = new RetryTemplate(); + BackOffPolicy backOffPolicy = this.loadBalancedRetryFactory + .createBackOffPolicy(serviceId); + retryTemplate.setBackOffPolicy( + backOffPolicy == null ? new NoBackOffPolicy() : backOffPolicy); + RetryListener[] retryListeners = this.loadBalancedRetryFactory + .createRetryListeners(serviceId); + if (retryListeners != null && retryListeners.length != 0) { + retryTemplate.setListeners(retryListeners); + } + + retryTemplate.setRetryPolicy(retryPolicy == null ? new NeverRetryPolicy() + : new InterceptorRetryPolicy(toHttpRequest(request), retryPolicy, + loadBalancerClient, serviceId)); + return retryTemplate; + } + + // Visible for Sleuth instrumentation + public Client getDelegate() { + return delegate; + } + + private HttpRequest toHttpRequest(Request request) { + return new HttpRequest() { + @Override + public HttpMethod getMethod() { + return HttpMethod.resolve(request.httpMethod().name()); + } + + @Override + public String getMethodValue() { + return getMethod().name(); + } + + @Override + public URI getURI() { + return URI.create(request.url()); + } + + @Override + public HttpHeaders getHeaders() { + Map> headers = new HashMap<>(); + Map> feignHeaders = request.headers(); + for (String key : feignHeaders.keySet()) { + headers.put(key, new ArrayList<>(feignHeaders.get(key))); + } + HttpHeaders httpHeaders = new HttpHeaders(); + httpHeaders.putAll(headers); + return httpHeaders; + } + }; + } + +} diff --git a/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/ribbon/FeignLoadBalancer.java b/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/ribbon/FeignLoadBalancer.java index cdedb138..1f02d8f0 100644 --- a/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/ribbon/FeignLoadBalancer.java +++ b/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/ribbon/FeignLoadBalancer.java @@ -26,7 +26,6 @@ import java.util.List; import java.util.Map; import com.netflix.client.AbstractLoadBalancerAwareClient; -import com.netflix.client.ClientException; import com.netflix.client.ClientRequest; import com.netflix.client.IResponse; import com.netflix.client.RequestSpecificRetryHandler; @@ -169,7 +168,7 @@ public class FeignLoadBalancer extends Map> feignHeaders = RibbonRequest.this .toRequest().headers(); for (String key : feignHeaders.keySet()) { - headers.put(key, new ArrayList(feignHeaders.get(key))); + headers.put(key, new ArrayList<>(feignHeaders.get(key))); } HttpHeaders httpHeaders = new HttpHeaders(); httpHeaders.putAll(headers); @@ -206,7 +205,7 @@ public class FeignLoadBalancer extends } @Override - public Object getPayload() throws ClientException { + public Object getPayload() { return this.response.body(); } diff --git a/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/ribbon/RetryableFeignLoadBalancer.java b/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/ribbon/RetryableFeignLoadBalancer.java index d068d875..0b67df7b 100644 --- a/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/ribbon/RetryableFeignLoadBalancer.java +++ b/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/ribbon/RetryableFeignLoadBalancer.java @@ -36,8 +36,6 @@ import org.springframework.cloud.client.loadbalancer.ServiceInstanceChooser; import org.springframework.cloud.netflix.ribbon.RibbonLoadBalancerClient; import org.springframework.cloud.netflix.ribbon.RibbonProperties; import org.springframework.cloud.netflix.ribbon.ServerIntrospector; -import org.springframework.retry.RetryCallback; -import org.springframework.retry.RetryContext; import org.springframework.retry.RetryListener; import org.springframework.retry.backoff.BackOffPolicy; import org.springframework.retry.backoff.NoBackOffPolicy; @@ -91,39 +89,34 @@ public class RetryableFeignLoadBalancer extends FeignLoadBalancer retryTemplate.setRetryPolicy(retryPolicy == null ? new NeverRetryPolicy() : new FeignRetryPolicy(request.toHttpRequest(), retryPolicy, this, this.getClientName())); - return retryTemplate.execute(new RetryCallback() { - @Override - public RibbonResponse doWithRetry(RetryContext retryContext) - throws IOException { - Request feignRequest = null; - // on retries the policy will choose the server and set it in the context - // extract the server and update the request being made - if (retryContext instanceof LoadBalancedRetryContext) { - ServiceInstance service = ((LoadBalancedRetryContext) retryContext) - .getServiceInstance(); - if (service != null) { - feignRequest = ((RibbonRequest) request - .replaceUri(reconstructURIWithServer( - new Server(service.getHost(), service.getPort()), - request.getUri()))).toRequest(); - } + return retryTemplate.execute(retryContext -> { + Request feignRequest = null; + // on retries the policy will choose the server and set it in the context + // extract the server and update the request being made + if (retryContext instanceof LoadBalancedRetryContext) { + ServiceInstance service = ((LoadBalancedRetryContext) retryContext) + .getServiceInstance(); + if (service != null) { + feignRequest = ((RibbonRequest) request + .replaceUri(reconstructURIWithServer( + new Server(service.getHost(), service.getPort()), + request.getUri()))).toRequest(); } - if (feignRequest == null) { - feignRequest = request.toRequest(); - } - Response response = request.client().execute(feignRequest, options); - if (retryPolicy != null - && retryPolicy.retryableStatusCode(response.status())) { - byte[] byteArray = response.body() == null ? new byte[] {} - : StreamUtils - .copyToByteArray(response.body().asInputStream()); - response.close(); - throw new RibbonResponseStatusCodeException( - RetryableFeignLoadBalancer.this.clientName, response, - byteArray, request.getUri()); - } - return new RibbonResponse(request.getUri(), response); } + if (feignRequest == null) { + feignRequest = request.toRequest(); + } + Response response = request.client().execute(feignRequest, options); + if (retryPolicy != null + && retryPolicy.retryableStatusCode(response.status())) { + byte[] byteArray = response.body() == null ? new byte[] {} + : StreamUtils.copyToByteArray(response.body().asInputStream()); + response.close(); + throw new RibbonResponseStatusCodeException( + RetryableFeignLoadBalancer.this.clientName, response, byteArray, + request.getUri()); + } + return new RibbonResponse(request.getUri(), response); }, new LoadBalancedRecoveryCallback() { @Override protected RibbonResponse createResponse(Response response, URI uri) { diff --git a/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/loadbalancer/FeignLoadBalancerAutoConfigurationTests.java b/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/loadbalancer/FeignLoadBalancerAutoConfigurationTests.java index 3849d25b..ca540972 100644 --- a/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/loadbalancer/FeignLoadBalancerAutoConfigurationTests.java +++ b/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/loadbalancer/FeignLoadBalancerAutoConfigurationTests.java @@ -45,7 +45,8 @@ class FeignLoadBalancerAutoConfigurationTests { void shouldInstantiateDefaultFeignBlockingLoadBalancerClientWhenHttpClientDisabled() { ConfigurableApplicationContext context = initContext( "spring.cloud.loadbalancer.ribbon.enabled=false", - "feign.httpclient.enabled=false"); + "feign.httpclient.enabled=false", + "spring.cloud.loadbalancer.retry.enabled=false"); assertThatOneBeanPresent(context, BlockingLoadBalancerClient.class); assertLoadBalanced(context, Client.Default.class); assertThatBeanNotPresent(context, LoadBalancerFeignClient.class); @@ -54,7 +55,8 @@ class FeignLoadBalancerAutoConfigurationTests { @Test void shouldInstantiateHttpFeignClientWhenEnabled() { ConfigurableApplicationContext context = initContext( - "spring.cloud.loadbalancer.ribbon.enabled=false"); + "spring.cloud.loadbalancer.ribbon.enabled=false", + "spring.cloud.loadbalancer.retry.enabled=false"); assertThatOneBeanPresent(context, BlockingLoadBalancerClient.class); assertLoadBalanced(context, ApacheHttpClient.class); assertThatBeanNotPresent(context, LoadBalancerFeignClient.class); @@ -64,16 +66,47 @@ class FeignLoadBalancerAutoConfigurationTests { void shouldInstantiateOkHttpFeignClientWhenEnabled() { ConfigurableApplicationContext context = initContext( "spring.cloud.loadbalancer.ribbon.enabled=false", - "feign.httpclient.enabled=false", "feign.okhttp.enabled=true"); + "feign.httpclient.enabled=false", "feign.okhttp.enabled=true", + "spring.cloud.loadbalancer.retry.enabled=false"); assertThatOneBeanPresent(context, BlockingLoadBalancerClient.class); assertLoadBalanced(context, OkHttpClient.class); assertThatBeanNotPresent(context, LoadBalancerFeignClient.class); } + @Test + void shouldInstantiateRetryableDefaultFeignBlockingLoadBalancerClientWhenHttpClientDisabled() { + ConfigurableApplicationContext context = initContext( + "spring.cloud.loadbalancer.ribbon.enabled=false", + "feign.httpclient.enabled=false"); + assertThatOneBeanPresent(context, BlockingLoadBalancerClient.class); + assertLoadBalancedWithRetries(context, Client.Default.class); + assertThatBeanNotPresent(context, LoadBalancerFeignClient.class); + } + + @Test + void shouldInstantiateRetryableHttpFeignClientWhenEnabled() { + ConfigurableApplicationContext context = initContext( + "spring.cloud.loadbalancer.ribbon.enabled=false"); + assertThatOneBeanPresent(context, BlockingLoadBalancerClient.class); + assertLoadBalancedWithRetries(context, ApacheHttpClient.class); + assertThatBeanNotPresent(context, LoadBalancerFeignClient.class); + } + + @Test + void shouldInstantiateRetryableOkHttpFeignClientWhenEnabled() { + ConfigurableApplicationContext context = initContext( + "spring.cloud.loadbalancer.ribbon.enabled=false", + "feign.httpclient.enabled=false", "feign.okhttp.enabled=true"); + assertThatOneBeanPresent(context, BlockingLoadBalancerClient.class); + assertLoadBalancedWithRetries(context, OkHttpClient.class); + assertThatBeanNotPresent(context, LoadBalancerFeignClient.class); + } + @Test void shouldNotProcessLoadBalancerConfigurationWhenRibbonEnabled() { ConfigurableApplicationContext context = initContext( - "spring.cloud.loadbalancer.ribbon.enabled=true"); + "spring.cloud.loadbalancer.ribbon.enabled=true", + "spring.cloud.loadbalancer.retry.enabled=false"); assertThatOneBeanPresent(context, LoadBalancerFeignClient.class); assertThatBeanNotPresent(context, BlockingLoadBalancerClient.class); assertThatBeanNotPresent(context, FeignBlockingLoadBalancerClient.class); @@ -104,6 +137,18 @@ class FeignLoadBalancerAutoConfigurationTests { assertThat(beans.get("feignClient").getDelegate()).isInstanceOf(delegateClass); } + private void assertLoadBalancedWithRetries(ConfigurableApplicationContext context, + Class delegateClass) { + Map retryableBeans = context + .getBeansOfType(RetryableBlockingFeignLoadBalancerClient.class); + assertThat(retryableBeans).hasSize(1); + Map beans = context + .getBeansOfType(FeignBlockingLoadBalancerClient.class); + assertThat(beans).isEmpty(); + assertThat(retryableBeans.get("feignRetryClient").getDelegate()) + .isInstanceOf(delegateClass); + } + private void assertThatBeanNotPresent(ConfigurableApplicationContext context, Class beanClass) { Map beans = context.getBeansOfType(beanClass); diff --git a/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/loadbalancer/RetryableBlockingFeignLoadBalancerClientTests.java b/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/loadbalancer/RetryableBlockingFeignLoadBalancerClientTests.java new file mode 100644 index 00000000..7d5d2e9f --- /dev/null +++ b/spring-cloud-openfeign-core/src/test/java/org/springframework/cloud/openfeign/loadbalancer/RetryableBlockingFeignLoadBalancerClientTests.java @@ -0,0 +1,197 @@ +/* + * Copyright 2013-2020 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.cloud.openfeign.loadbalancer; + +import java.io.IOException; +import java.net.URI; +import java.nio.charset.StandardCharsets; +import java.util.Collection; +import java.util.Collections; +import java.util.HashMap; +import java.util.Map; + +import feign.Client; +import feign.Request; +import feign.Response; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.ArgumentCaptor; +import org.mockito.junit.jupiter.MockitoExtension; + +import org.springframework.cloud.client.DefaultServiceInstance; +import org.springframework.cloud.client.ServiceInstance; +import org.springframework.cloud.client.loadbalancer.LoadBalancedRetryFactory; +import org.springframework.cloud.client.loadbalancer.LoadBalancerRetryProperties; +import org.springframework.cloud.loadbalancer.blocking.client.BlockingLoadBalancerClient; +import org.springframework.cloud.loadbalancer.blocking.retry.BlockingLoadBalancedRetryPolicy; +import org.springframework.http.HttpHeaders; +import org.springframework.http.MediaType; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.argThat; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +/** + * Tests for {@link RetryableBlockingFeignLoadBalancerClient}. Note: the underlying + * {@link BlockingLoadBalancerClient} is already extensively tested in the Spring Cloud + * Commons project, so here we are only testing the interactions between + * {@link RetryableBlockingFeignLoadBalancerClient} and its delegates. + * + * @see BlockingLoadBalancerClientTests + * @author Olga Maciaszek-Sharma + */ +@ExtendWith(MockitoExtension.class) +class RetryableBlockingFeignLoadBalancerClientTests { + + private Client delegate = mock(Client.class); + + private LoadBalancedRetryFactory retryFactory = mock(LoadBalancedRetryFactory.class); + + private BlockingLoadBalancerClient loadBalancerClient = mock( + BlockingLoadBalancerClient.class); + + private LoadBalancerRetryProperties properties = new LoadBalancerRetryProperties(); + + private RetryableBlockingFeignLoadBalancerClient feignBlockingLoadBalancerClient = new RetryableBlockingFeignLoadBalancerClient( + delegate, loadBalancerClient, retryFactory); + + private ServiceInstance serviceInstance = new DefaultServiceInstance("test-a", "test", + "testhost", 80, false); + + @BeforeEach + void setUp() { + when(retryFactory.createRetryPolicy(any(), eq(loadBalancerClient))) + .thenReturn(new BlockingLoadBalancedRetryPolicy("test", + loadBalancerClient, properties)); + when(loadBalancerClient.choose("test")).thenReturn(serviceInstance); + } + + @Test + void shouldExtractServiceIdFromRequestUrl() throws IOException { + Request request = testRequest(); + Response response = testResponse(200); + when(delegate.execute(any(), any())).thenReturn(response); + when(retryFactory.createRetryPolicy(any(), eq(loadBalancerClient))) + .thenReturn(new BlockingLoadBalancedRetryPolicy("test", + loadBalancerClient, properties)); + when(loadBalancerClient.reconstructURI(serviceInstance, + URI.create("http://test/path"))) + .thenReturn(URI.create("http://testhost:80/path")); + + feignBlockingLoadBalancerClient.execute(request, new Request.Options()); + + verify(loadBalancerClient).choose("test"); + verify(loadBalancerClient).reconstructURI(serviceInstance, + URI.create("http://test/path")); + + verify(delegate).execute(argThat((Request actualRequest) -> actualRequest.url() + .equals("http://testhost:80/path")), any()); + } + + private Response testResponse(int status) { + return Response.builder().request(testRequest()).status(status).build(); + } + + @Test + void shouldExecuteOriginalRequestIfInstanceNotFound() throws IOException { + Request request = testRequest(); + Response response = testResponse(503); + when(loadBalancerClient.choose("test")).thenReturn(null); + when(delegate.execute(any(), any())).thenReturn(response); + when(retryFactory.createRetryPolicy(any(), eq(loadBalancerClient))) + .thenReturn(new BlockingLoadBalancedRetryPolicy("test", + loadBalancerClient, properties)); + + feignBlockingLoadBalancerClient.execute(request, new Request.Options()); + + verify(delegate).execute(eq(request), any()); + } + + @Test + void shouldRetryOnRepeatableStatusCode() throws IOException { + properties.getRetryableStatusCodes().add(503); + Request request = testRequest(); + Response response = testResponse(503); + when(delegate.execute(any(), any())).thenReturn(response); + when(retryFactory.createRetryPolicy(any(), eq(loadBalancerClient))) + .thenReturn(new BlockingLoadBalancedRetryPolicy("test", + loadBalancerClient, properties)); + when(loadBalancerClient.reconstructURI(serviceInstance, + URI.create("http://test/path"))) + .thenReturn(URI.create("http://testhost:80/path")); + + feignBlockingLoadBalancerClient.execute(request, new Request.Options()); + + verify(loadBalancerClient, times(2)).reconstructURI(serviceInstance, + URI.create("http://test/path")); + verify(delegate, times(2)).execute(any(), any()); + } + + @Test + void shouldPassCorrectRequestToDelegate() throws IOException { + Request request = testRequest(); + Request.Options options = new Request.Options(); + String url = "http://127.0.0.1/path"; + ServiceInstance serviceInstance = new DefaultServiceInstance("test-1", "test", + "test-host", 8888, false); + when(loadBalancerClient.choose("test")).thenReturn(serviceInstance); + when(loadBalancerClient.reconstructURI(serviceInstance, + URI.create("http://test/path"))).thenReturn(URI.create(url)); + Response response = testResponse(200); + when(delegate.execute(any(), any())).thenReturn(response); + when(retryFactory.createRetryPolicy(any(), eq(loadBalancerClient))) + .thenReturn(new BlockingLoadBalancedRetryPolicy("test", + loadBalancerClient, properties)); + + feignBlockingLoadBalancerClient.execute(request, options); + + ArgumentCaptor captor = ArgumentCaptor.forClass(Request.class); + verify(delegate, times(1)).execute(captor.capture(), eq(options)); + Request actualRequest = captor.getValue(); + assertThat(actualRequest.httpMethod()).isEqualTo(Request.HttpMethod.GET); + assertThat(actualRequest.url()).isEqualTo(url); + assertThat(actualRequest.headers()).hasSize(1); + assertThat(actualRequest.headers()).containsEntry(HttpHeaders.CONTENT_TYPE, + Collections.singletonList(MediaType.APPLICATION_JSON_VALUE)); + assertThat(new String(actualRequest.body())).isEqualTo("hello"); + } + + private Request testRequest() { + return testRequest("test"); + } + + private Request testRequest(String host) { + return Request.create(Request.HttpMethod.GET, "http://" + host + "/path", + testHeaders(), "hello".getBytes(), StandardCharsets.UTF_8, null); + } + + private Map> testHeaders() { + Map> feignHeaders = new HashMap<>(); + feignHeaders.put(HttpHeaders.CONTENT_TYPE, + Collections.singletonList(MediaType.APPLICATION_JSON_VALUE)); + return feignHeaders; + + } + +}