From 7d06c140fa319f7bbdbc14ae77c4f7b9647a7bb3 Mon Sep 17 00:00:00 2001 From: Ryan Baxter Date: Mon, 5 Jun 2017 14:36:49 -0400 Subject: [PATCH] Close response when retry on certain status codes. Fixes #1970. --- .../apache/RetryableRibbonLoadBalancingHttpClient.java | 4 ++++ .../okhttp/RetryableOkHttpLoadBalancingClient.java | 1 + .../apache/RibbonLoadBalancingHttpClientTests.java | 10 +++++++--- 3 files changed, 12 insertions(+), 3 deletions(-) diff --git a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/apache/RetryableRibbonLoadBalancingHttpClient.java b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/apache/RetryableRibbonLoadBalancingHttpClient.java index ebf35907..a46eb011 100644 --- a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/apache/RetryableRibbonLoadBalancingHttpClient.java +++ b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/apache/RetryableRibbonLoadBalancingHttpClient.java @@ -20,6 +20,7 @@ import java.net.URI; import org.apache.commons.lang.BooleanUtils; import org.apache.http.HttpResponse; import org.apache.http.client.config.RequestConfig; +import org.apache.http.client.methods.CloseableHttpResponse; import org.apache.http.client.methods.HttpUriRequest; import org.springframework.cloud.client.ServiceInstance; import org.springframework.cloud.client.loadbalancer.LoadBalancedRetryContext; @@ -91,6 +92,9 @@ public class RetryableRibbonLoadBalancingHttpClient extends RibbonLoadBalancingH HttpUriRequest httpUriRequest = newRequest.toRequest(requestConfig); final HttpResponse httpResponse = RetryableRibbonLoadBalancingHttpClient.this.delegate.execute(httpUriRequest); if(retryPolicy.retryableStatusCode(httpResponse.getStatusLine().getStatusCode())) { + if(CloseableHttpResponse.class.isInstance(httpResponse)) { + ((CloseableHttpResponse)httpResponse).close(); + } throw new RetryableStatusCodeException(RetryableRibbonLoadBalancingHttpClient.this.clientName, httpResponse.getStatusLine().getStatusCode()); } diff --git a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/okhttp/RetryableOkHttpLoadBalancingClient.java b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/okhttp/RetryableOkHttpLoadBalancingClient.java index a61f2a74..170c275a 100644 --- a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/okhttp/RetryableOkHttpLoadBalancingClient.java +++ b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/okhttp/RetryableOkHttpLoadBalancingClient.java @@ -96,6 +96,7 @@ public class RetryableOkHttpLoadBalancingClient extends OkHttpLoadBalancingClien final Request request = newRequest.toRequest(); Response response = httpClient.newCall(request).execute(); if(retryPolicy.retryableStatusCode(response.code())) { + response.close(); throw new RetryableStatusCodeException(RetryableOkHttpLoadBalancingClient.this.clientName, response.code()); } return new OkHttpRibbonResponse(response, newRequest.getUri()); diff --git a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/ribbon/apache/RibbonLoadBalancingHttpClientTests.java b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/ribbon/apache/RibbonLoadBalancingHttpClientTests.java index 266b39be..b77318f0 100644 --- a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/ribbon/apache/RibbonLoadBalancingHttpClientTests.java +++ b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/ribbon/apache/RibbonLoadBalancingHttpClientTests.java @@ -22,6 +22,7 @@ import org.apache.http.HttpResponse; import org.apache.http.StatusLine; import org.apache.http.client.HttpClient; import org.apache.http.client.config.RequestConfig; +import org.apache.http.client.methods.CloseableHttpResponse; import org.apache.http.client.methods.HttpUriRequest; import org.apache.http.impl.conn.PoolingHttpClientConnectionManager; import org.junit.After; @@ -285,7 +286,7 @@ public class RibbonLoadBalancingHttpClientTests { HttpMethod method = HttpMethod.POST; URI uri = new URI("http://" + host + ":" + port); HttpClient delegate = mock(HttpClient.class); - final HttpResponse response = mock(HttpResponse.class); + final CloseableHttpResponse response = mock(CloseableHttpResponse.class); StatusLine statusLine = mock(StatusLine.class); doReturn(200).when(statusLine).getStatusCode(); doReturn(statusLine).when(response).getStatusLine(); @@ -301,6 +302,7 @@ public class RibbonLoadBalancingHttpClientTests { HttpUriRequest uriRequest = mock(HttpUriRequest.class); doReturn(uriRequest).when(request).toRequest(any(RequestConfig.class)); RibbonApacheHttpResponse returnedResponse = client.execute(request, null); + verify(response, times(0)).close(); verify(delegate, times(3)).execute(any(HttpUriRequest.class)); verify(lb, times(1)).chooseServer(eq(serviceName)); } @@ -317,7 +319,7 @@ public class RibbonLoadBalancingHttpClientTests { HttpMethod method = HttpMethod.POST; URI uri = new URI("http://" + host + ":" + port); HttpClient delegate = mock(HttpClient.class); - final HttpResponse response = mock(HttpResponse.class); + final CloseableHttpResponse response = mock(CloseableHttpResponse.class); doThrow(new IOException("boom")).doThrow(new IOException("boom again")).doReturn(response). when(delegate).execute(any(HttpUriRequest.class)); ILoadBalancer lb = mock(ILoadBalancer.class); @@ -334,6 +336,7 @@ public class RibbonLoadBalancingHttpClientTests { client.execute(request, null); fail("Expected IOException"); } catch(IOException e) {} finally { + verify(response, times(0)).close(); verify(delegate, times(1)).execute(any(HttpUriRequest.class)); verify(lb, times(0)).chooseServer(eq(serviceName)); } @@ -355,7 +358,7 @@ public class RibbonLoadBalancingHttpClientTests { StatusLine statusLine = mock(StatusLine.class); doReturn(200).when(statusLine).getStatusCode(); doReturn(statusLine).when(response).getStatusLine(); - final HttpResponse fourOFourResponse = mock(HttpResponse.class); + final CloseableHttpResponse fourOFourResponse = mock(CloseableHttpResponse.class); StatusLine fourOFourStatusLine = mock(StatusLine.class); doReturn(404).when(fourOFourStatusLine).getStatusCode(); doReturn(fourOFourStatusLine).when(fourOFourResponse).getStatusLine(); @@ -371,6 +374,7 @@ public class RibbonLoadBalancingHttpClientTests { doReturn(uri).when(uriRequest).getURI(); doReturn(uriRequest).when(request).toRequest(any(RequestConfig.class)); RibbonApacheHttpResponse returnedResponse = client.execute(request, null); + verify(fourOFourResponse, times(1)).close(); verify(delegate, times(2)).execute(any(HttpUriRequest.class)); verify(lb, times(0)).chooseServer(eq(serviceName)); }