Repository: spring-cloud/spring-cloud-commons
Issue: RetryTemplate does not work properly when calling multiple serviceNames

Hi guys.

We encountered following issue when using consul and `RetryLoadBalancerInterceptor`

Service-A  calls service-1 and service-2.
Service-1 and service-2 are provided in different servers with different paths.
service-1 : http://host-1:8888/path1/service1
service-2: http://host-2:9999/path2/service2

But we found that some requests access following URLs which caused 404 errors.
http://host-2:9999/path1/service1
http://host-1:8888/path2/service2

We researched the source code and found that in `RetryLoadBalancerInterceptor` since the retryTemplate is singleton, the retryPolicy inside retryTemplate maybe modified by another thread before the request is executed.

<pre><code>retryTemplate.setRetryPolicy(
  !lbProperties.isEnabled() || retryPolicy == null ? new NeverRetryPolicy()
	 : new InterceptorRetryPolicy(request, retryPolicy, loadBalancer,
		 serviceName));
</code></pre>

Please see the code in `RetryTemplate` at about line 277 (master branch)
 `while (canRetry(retryPolicy, context) && !context.isExhaustedOnly()) `

And `InterceptorRetryPolicy` `canRetry` method, 
<pre><code> LoadBalancedRetryContext lbContext = (LoadBalancedRetryContext)context;
        if(lbContext.getRetryCount() == 0  && lbContext.getServiceInstance() == null) {
            //We haven't even tried to make the request yet so return true so we do
            lbContext.setServiceInstance(serviceInstanceChooser.choose(serviceName));
            return true;
        }
</code></pre>

Since the serviceInstance which contains the real service host and port info maybe modified in canRetry method, it may change the request URL to a host of another serviceName (If another thread modified the retryPolicy with other serviceName. In class `ServiceRequestWrapper` is where the request URL is actually changed by using `ServiceInstance`).

 So we disabled the retryPolicy by config
`spring.cloud.loadbalancer.retry.enabled=false`
so that in `RetryLoadBalancerInterceptor.intercept` we will always use `NeverRetryPolicy` object, and we have not encountered 404 since then. (We encountered 404 every hour before modifying the configuration).

Would you please confirm if my investigation on our issue is correct or not?

Thank you.







 

Changed files:
spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/RetryLoadBalancerInterceptor.java
spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/RetryLoadBalancerInterceptorTest.java
spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/LoadBalancerAutoConfiguration.java

diff --git a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/LoadBalancerAutoConfiguration.java b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/LoadBalancerAutoConfiguration.java
index 02257ac0..9f4cb42d 100644
--- a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/LoadBalancerAutoConfiguration.java
+++ b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/LoadBalancerAutoConfiguration.java
@@ -126,8 +126,8 @@ public class LoadBalancerAutoConfiguration {
 		public RetryLoadBalancerInterceptor ribbonInterceptor(
 				LoadBalancerClient loadBalancerClient, LoadBalancerRetryProperties properties,
 				LoadBalancedRetryPolicyFactory lbRetryPolicyFactory,
-				LoadBalancerRequestFactory requestFactory, RetryTemplate retryTemplate) {
-			return new RetryLoadBalancerInterceptor(loadBalancerClient, retryTemplate, properties,
+				LoadBalancerRequestFactory requestFactory) {
+			return new RetryLoadBalancerInterceptor(loadBalancerClient, properties,
 					lbRetryPolicyFactory, requestFactory);
 		}
 
diff --git a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/RetryLoadBalancerInterceptor.java b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/RetryLoadBalancerInterceptor.java
index d632b471..cabf8d45 100644
--- a/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/RetryLoadBalancerInterceptor.java
+++ b/spring-cloud-commons/src/main/java/org/springframework/cloud/client/loadbalancer/RetryLoadBalancerInterceptor.java
@@ -42,7 +42,7 @@ public class RetryLoadBalancerInterceptor implements ClientHttpRequestIntercepto
 	private LoadBalancerRetryProperties lbProperties;
 	private LoadBalancerRequestFactory requestFactory;
 
-
+	@Deprecated
 	public RetryLoadBalancerInterceptor(LoadBalancerClient loadBalancer, RetryTemplate retryTemplate,
 										LoadBalancerRetryProperties lbProperties,
 										LoadBalancedRetryPolicyFactory lbRetryPolicyFactory,
@@ -53,7 +53,18 @@ public class RetryLoadBalancerInterceptor implements ClientHttpRequestIntercepto
 		this.lbProperties = lbProperties;
 		this.requestFactory = requestFactory;
 	}
-	
+
+	public RetryLoadBalancerInterceptor(LoadBalancerClient loadBalancer,
+										LoadBalancerRetryProperties lbProperties,
+										LoadBalancedRetryPolicyFactory lbRetryPolicyFactory,
+										LoadBalancerRequestFactory requestFactory) {
+		this.loadBalancer = loadBalancer;
+		this.lbRetryPolicyFactory = lbRetryPolicyFactory;
+		this.lbProperties = lbProperties;
+		this.requestFactory = requestFactory;
+	}
+
+	@Deprecated
 	public RetryLoadBalancerInterceptor(LoadBalancerClient loadBalancer, RetryTemplate retryTemplate,
 										LoadBalancerRetryProperties lbProperties,
 										LoadBalancedRetryPolicyFactory lbRetryPolicyFactory) {
@@ -70,11 +81,13 @@ public class RetryLoadBalancerInterceptor implements ClientHttpRequestIntercepto
 		Assert.state(serviceName != null, "Request URI does not contain a valid hostname: " + originalUri);
 		final LoadBalancedRetryPolicy retryPolicy = lbRetryPolicyFactory.create(serviceName,
 				loadBalancer);
-		retryTemplate.setRetryPolicy(
+		RetryTemplate template = this.retryTemplate == null ? new RetryTemplate() : this.retryTemplate;
+		template.setThrowLastExceptionOnExhausted(true);
+		template.setRetryPolicy(
 				!lbProperties.isEnabled() || retryPolicy == null ? new NeverRetryPolicy()
 						: new InterceptorRetryPolicy(request, retryPolicy, loadBalancer,
 						serviceName));
-		return retryTemplate
+		return template
 				.execute(new RetryCallback<ClientHttpResponse, IOException>() {
 					@Override
 					public ClientHttpResponse doWithRetry(RetryContext context)
diff --git a/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/RetryLoadBalancerInterceptorTest.java b/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/RetryLoadBalancerInterceptorTest.java
index aaeaa768..307ee8d6 100644
--- a/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/RetryLoadBalancerInterceptorTest.java
+++ b/spring-cloud-commons/src/test/java/org/springframework/cloud/client/loadbalancer/RetryLoadBalancerInterceptorTest.java
@@ -33,14 +33,12 @@ import static org.mockito.Mockito.when;
 public class RetryLoadBalancerInterceptorTest {
 
     private LoadBalancerClient client;
-    private RetryTemplate retryTemplate;
     private LoadBalancerRetryProperties lbProperties;
     private LoadBalancerRequestFactory lbRequestFactory;
 
     @Before
     public void setUp() throws Exception {
         client = mock(LoadBalancerClient.class);
-        retryTemplate = spy(new RetryTemplate());
         lbProperties = new LoadBalancerRetryProperties();
         lbRequestFactory = mock(LoadBalancerRequestFactory.class);
 
@@ -49,7 +47,6 @@ public class RetryLoadBalancerInterceptorTest {
     @After
     public void tearDown() throws Exception {
         client = null;
-        retryTemplate = null;
         lbProperties = null;
     }
 
@@ -64,11 +61,10 @@ public class RetryLoadBalancerInterceptorTest {
         when(client.choose(eq("foo"))).thenReturn(serviceInstance);
         when(client.execute(eq("foo"), eq(serviceInstance), any(LoadBalancerRequest.class))).thenThrow(new IOException());
         lbProperties.setEnabled(false);
-        RetryLoadBalancerInterceptor interceptor = new RetryLoadBalancerInterceptor(client, retryTemplate, lbProperties, lbRetryPolicyFactory, lbRequestFactory);
+        RetryLoadBalancerInterceptor interceptor = new RetryLoadBalancerInterceptor(client, lbProperties, lbRetryPolicyFactory, lbRequestFactory);
         byte[] body = new byte[]{};
         ClientHttpRequestExecution execution = mock(ClientHttpRequestExecution.class);
         interceptor.intercept(request, body, execution);
-        verify(retryTemplate, times(1)).setRetryPolicy(any(NeverRetryPolicy.class));
         verify(lbRequestFactory).createRequest(request, body, execution);
     }
 
@@ -85,7 +81,7 @@ public class RetryLoadBalancerInterceptorTest {
         when(client.choose(eq("foo_underscore"))).thenReturn(serviceInstance);
         when(client.execute(eq("foo_underscore"), eq(serviceInstance), any(LoadBalancerRequest.class))).thenReturn(clientHttpResponse);
         lbProperties.setEnabled(true);
-        RetryLoadBalancerInterceptor interceptor = new RetryLoadBalancerInterceptor(client, retryTemplate, lbProperties, lbRetryPolicyFactory);
+        RetryLoadBalancerInterceptor interceptor = new RetryLoadBalancerInterceptor(client, lbProperties, lbRetryPolicyFactory, lbRequestFactory);
         byte[] body = new byte[]{};
         ClientHttpRequestExecution execution = mock(ClientHttpRequestExecution.class);
         ClientHttpResponse rsp = interceptor.intercept(request, body, execution);
@@ -102,11 +98,10 @@ public class RetryLoadBalancerInterceptorTest {
         when(client.choose(eq("foo"))).thenReturn(serviceInstance);
         when(client.execute(eq("foo"), eq(serviceInstance), any(LoadBalancerRequest.class))).thenReturn(clientHttpResponse);
         lbProperties.setEnabled(true);
-        RetryLoadBalancerInterceptor interceptor = new RetryLoadBalancerInterceptor(client, retryTemplate, lbProperties, lbRetryPolicyFactory, lbRequestFactory);
+        RetryLoadBalancerInterceptor interceptor = new RetryLoadBalancerInterceptor(client, lbProperties, lbRetryPolicyFactory, lbRequestFactory);
         byte[] body = new byte[]{};
         ClientHttpRequestExecution execution = mock(ClientHttpRequestExecution.class);
         interceptor.intercept(request, body, execution);
-        verify(retryTemplate, times(1)).setRetryPolicy(any(NeverRetryPolicy.class));
         verify(lbRequestFactory).createRequest(request, body, execution);
     }
 
@@ -123,12 +118,11 @@ public class RetryLoadBalancerInterceptorTest {
         when(client.choose(eq("foo"))).thenReturn(serviceInstance);
         when(client.execute(eq("foo"), eq(serviceInstance), any(LoadBalancerRequest.class))).thenReturn(clientHttpResponse);
         lbProperties.setEnabled(true);
-        RetryLoadBalancerInterceptor interceptor = new RetryLoadBalancerInterceptor(client, retryTemplate, lbProperties, lbRetryPolicyFactory, lbRequestFactory);
+        RetryLoadBalancerInterceptor interceptor = new RetryLoadBalancerInterceptor(client, lbProperties, lbRetryPolicyFactory, lbRequestFactory);
         byte[] body = new byte[]{};
         ClientHttpRequestExecution execution = mock(ClientHttpRequestExecution.class);
         ClientHttpResponse rsp = interceptor.intercept(request, body, execution);
         assertThat(rsp, is(clientHttpResponse));
-        verify(retryTemplate, times(1)).setRetryPolicy(eq(interceptorRetryPolicy));
         verify(lbRequestFactory).createRequest(request, body, execution);
     }
 
@@ -149,13 +143,12 @@ public class RetryLoadBalancerInterceptorTest {
         when(client.execute(eq("foo"), eq(serviceInstance), any(LoadBalancerRequest.class))).
                 thenReturn(clientHttpResponseNotFound).thenReturn(clientHttpResponseOk);
         lbProperties.setEnabled(true);
-        RetryLoadBalancerInterceptor interceptor = new RetryLoadBalancerInterceptor(client, retryTemplate, lbProperties, lbRetryPolicyFactory, lbRequestFactory);
+        RetryLoadBalancerInterceptor interceptor = new RetryLoadBalancerInterceptor(client, lbProperties, lbRetryPolicyFactory, lbRequestFactory);
         byte[] body = new byte[]{};
         ClientHttpRequestExecution execution = mock(ClientHttpRequestExecution.class);
         ClientHttpResponse rsp = interceptor.intercept(request, body, execution);
         verify(client, times(2)).execute(eq("foo"), eq(serviceInstance), any(LoadBalancerRequest.class));
         assertThat(rsp, is(clientHttpResponseOk));
-        verify(retryTemplate, times(1)).setRetryPolicy(eq(interceptorRetryPolicy));
         verify(lbRequestFactory, times(2)).createRequest(request, body, execution);
     }
 
@@ -172,13 +165,12 @@ public class RetryLoadBalancerInterceptorTest {
         when(client.choose(eq("foo"))).thenReturn(serviceInstance);
         when(client.execute(eq("foo"), eq(serviceInstance), any(LoadBalancerRequest.class))).thenThrow(new IOException()).thenReturn(clientHttpResponse);
         lbProperties.setEnabled(true);
-        RetryLoadBalancerInterceptor interceptor = new RetryLoadBalancerInterceptor(client, retryTemplate, lbProperties, lbRetryPolicyFactory, lbRequestFactory);
+        RetryLoadBalancerInterceptor interceptor = new RetryLoadBalancerInterceptor(client, lbProperties, lbRetryPolicyFactory, lbRequestFactory);
         byte[] body = new byte[]{};
         ClientHttpRequestExecution execution = mock(ClientHttpRequestExecution.class);
         ClientHttpResponse rsp = interceptor.intercept(request, body, execution);
         verify(client, times(2)).execute(eq("foo"), eq(serviceInstance), any(LoadBalancerRequest.class));
         assertThat(rsp, is(clientHttpResponse));
-        verify(retryTemplate, times(1)).setRetryPolicy(any(InterceptorRetryPolicy.class));
         verify(lbRequestFactory, times(2)).createRequest(request, body, execution);
     }
 
@@ -196,7 +188,7 @@ public class RetryLoadBalancerInterceptorTest {
         when(client.choose(eq("foo"))).thenReturn(serviceInstance);
         when(client.execute(eq("foo"), eq(serviceInstance), any(LoadBalancerRequest.class))).thenThrow(new IOException()).thenReturn(clientHttpResponse);
         lbProperties.setEnabled(true);
-        RetryLoadBalancerInterceptor interceptor = new RetryLoadBalancerInterceptor(client, retryTemplate, lbProperties, lbRetryPolicyFactory, lbRequestFactory);
+        RetryLoadBalancerInterceptor interceptor = new RetryLoadBalancerInterceptor(client, lbProperties, lbRetryPolicyFactory, lbRequestFactory);
         byte[] body = new byte[]{};
         ClientHttpRequestExecution execution = mock(ClientHttpRequestExecution.class);
         ClientHttpResponse rsp = interceptor.intercept(request, body, execution);