You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@hc.apache.org by ol...@apache.org on 2011/12/23 21:59:54 UTC
svn commit: r1222845 - in /httpcomponents/httpclient/trunk: ./
httpclient-cache/src/main/java/org/apache/http/impl/client/cache/
httpclient-cache/src/test/java/org/apache/http/impl/client/cache/
httpclient/src/main/java/org/apache/http/impl/client/
Author: olegk
Date: Fri Dec 23 20:59:54 2011
New Revision: 1222845
URL: http://svn.apache.org/viewvc?rev=1222845&view=rev
Log:
HTTPCLIENT-1155: CachingHttpClient fails to ensure that the response content gets fully consumed when using a ResponseHandler, which can potentially lead to connection leaks
Contributed by James Miller <jamesmiller01 at gmail dot com>
Modified:
httpcomponents/httpclient/trunk/RELEASE_NOTES.txt
httpcomponents/httpclient/trunk/httpclient-cache/src/main/java/org/apache/http/impl/client/cache/CachingHttpClient.java
httpcomponents/httpclient/trunk/httpclient-cache/src/test/java/org/apache/http/impl/client/cache/TestCachingHttpClient.java
httpcomponents/httpclient/trunk/httpclient/src/main/java/org/apache/http/impl/client/AbstractHttpClient.java
Modified: httpcomponents/httpclient/trunk/RELEASE_NOTES.txt
URL: http://svn.apache.org/viewvc/httpcomponents/httpclient/trunk/RELEASE_NOTES.txt?rev=1222845&r1=1222844&r2=1222845&view=diff
==============================================================================
--- httpcomponents/httpclient/trunk/RELEASE_NOTES.txt (original)
+++ httpcomponents/httpclient/trunk/RELEASE_NOTES.txt Fri Dec 23 20:59:54 2011
@@ -1,6 +1,10 @@
Changes since 4.2 ALPHA1
-------------------
+* [HTTPCLIENT-1155] CachingHttpClient fails to ensure that the response content gets fully consumed
+ when using a ResponseHandler, which can potentially lead to connection leaks.
+ Contributed by James Miller <jamesmiller01 at gmail dot com>
+
* [HTTPCLIENT-1147] When HttpClient-Cache cannot open cache file, should act like miss.
Contributed by Joe Campbell <joseph.r.campbell at gmail.com>
Modified: httpcomponents/httpclient/trunk/httpclient-cache/src/main/java/org/apache/http/impl/client/cache/CachingHttpClient.java
URL: http://svn.apache.org/viewvc/httpcomponents/httpclient/trunk/httpclient-cache/src/main/java/org/apache/http/impl/client/cache/CachingHttpClient.java?rev=1222845&r1=1222844&r2=1222845&view=diff
==============================================================================
--- httpcomponents/httpclient/trunk/httpclient-cache/src/main/java/org/apache/http/impl/client/cache/CachingHttpClient.java (original)
+++ httpcomponents/httpclient/trunk/httpclient-cache/src/main/java/org/apache/http/impl/client/cache/CachingHttpClient.java Fri Dec 23 20:59:54 2011
@@ -27,6 +27,7 @@
package org.apache.http.impl.client.cache;
import java.io.IOException;
+import java.lang.reflect.UndeclaredThrowableException;
import java.net.URI;
import java.util.Date;
import java.util.HashMap;
@@ -337,7 +338,7 @@ public class CachingHttpClient implement
public <T> T execute(HttpHost target, HttpRequest request,
ResponseHandler<? extends T> responseHandler, HttpContext context) throws IOException {
HttpResponse resp = execute(target, request, context);
- return responseHandler.handleResponse(resp);
+ return handleAndConsume(responseHandler,resp);
}
public HttpResponse execute(HttpUriRequest request) throws IOException {
@@ -359,7 +360,38 @@ public class CachingHttpClient implement
public <T> T execute(HttpUriRequest request, ResponseHandler<? extends T> responseHandler,
HttpContext context) throws IOException {
HttpResponse resp = execute(request, context);
- return responseHandler.handleResponse(resp);
+ return handleAndConsume(responseHandler, resp);
+ }
+
+ private <T> T handleAndConsume(
+ final ResponseHandler<? extends T> responseHandler,
+ HttpResponse response) throws Error, IOException {
+ T result;
+ try {
+ result = responseHandler.handleResponse(response);
+ } catch (Exception t) {
+ HttpEntity entity = response.getEntity();
+ try {
+ EntityUtils.consume(entity);
+ } catch (Exception t2) {
+ // Log this exception. The original exception is more
+ // important and will be thrown to the caller.
+ this.log.warn("Error consuming content after an exception.", t2);
+ }
+ if (t instanceof RuntimeException) {
+ throw (RuntimeException) t;
+ }
+ if (t instanceof IOException) {
+ throw (IOException) t;
+ }
+ throw new UndeclaredThrowableException(t);
+ }
+
+ // Handling the response was successful. Ensure that the content has
+ // been fully consumed.
+ HttpEntity entity = response.getEntity();
+ EntityUtils.consume(entity);
+ return result;
}
public ClientConnectionManager getConnectionManager() {
Modified: httpcomponents/httpclient/trunk/httpclient-cache/src/test/java/org/apache/http/impl/client/cache/TestCachingHttpClient.java
URL: http://svn.apache.org/viewvc/httpcomponents/httpclient/trunk/httpclient-cache/src/test/java/org/apache/http/impl/client/cache/TestCachingHttpClient.java?rev=1222845&r1=1222844&r2=1222845&view=diff
==============================================================================
--- httpcomponents/httpclient/trunk/httpclient-cache/src/test/java/org/apache/http/impl/client/cache/TestCachingHttpClient.java (original)
+++ httpcomponents/httpclient/trunk/httpclient-cache/src/test/java/org/apache/http/impl/client/cache/TestCachingHttpClient.java Fri Dec 23 20:59:54 2011
@@ -26,6 +26,7 @@
*/
package org.apache.http.impl.client.cache;
+import java.io.ByteArrayInputStream;
import java.io.IOException;
import java.net.URI;
import java.util.ArrayList;
@@ -34,6 +35,7 @@ import java.util.HashMap;
import java.util.List;
import java.util.Map;
import org.apache.http.Header;
+import org.apache.http.HttpEntity;
import org.apache.http.HttpHost;
import org.apache.http.HttpRequest;
import org.apache.http.HttpResponse;
@@ -50,6 +52,7 @@ import org.apache.http.client.cache.Http
import org.apache.http.client.methods.HttpGet;
import org.apache.http.client.methods.HttpUriRequest;
import org.apache.http.conn.ClientConnectionManager;
+import org.apache.http.entity.InputStreamEntity;
import org.apache.http.impl.cookie.DateUtils;
import org.apache.http.message.BasicHeader;
import org.apache.http.message.BasicHttpRequest;
@@ -106,6 +109,7 @@ public class TestCachingHttpClient {
private HttpContext context;
private HttpParams params;
private HttpCacheEntry entry;
+ private ConsumableInputStream cis;
@SuppressWarnings("unchecked")
@Before
@@ -620,12 +624,61 @@ public class TestCachingHttpClient {
expect(mockHandler.handleResponse(mockBackendResponse)).andReturn(
theObject);
-
+ HttpEntity streamingEntity = makeStreamingEntity();
+ expect(theResponse.getEntity()).andReturn(streamingEntity);
replayMocks();
Object result = impl.execute(host, request, mockHandler, context);
verifyMocks();
Assert.assertEquals(1, c.getCount());
Assert.assertSame(theObject, result);
+ Assert.assertTrue(cis.wasClosed());
+ }
+
+ @Test
+ public void testConsumesEntityOnExecuteWithException() throws Exception {
+
+ final Counter c = new Counter();
+ final HttpHost theHost = host;
+ final HttpRequest theRequest = request;
+ final HttpResponse theResponse = mockBackendResponse;
+ final HttpContext theContext = context;
+ impl = new CachingHttpClient(
+ mockBackend,
+ mockValidityPolicy,
+ mockResponsePolicy,
+ mockCache,
+ mockResponseGenerator,
+ mockRequestPolicy,
+ mockSuitabilityChecker,
+ mockConditionalRequestBuilder,
+ mockResponseProtocolCompliance,
+ mockRequestProtocolCompliance) {
+ @Override
+ public HttpResponse execute(HttpHost target, HttpRequest request, HttpContext context) {
+ Assert.assertSame(theHost, target);
+ Assert.assertSame(theRequest, request);
+ Assert.assertSame(theContext, context);
+ c.incr();
+ return theResponse;
+ }
+ };
+ IOException ioException = new IOException("test exception");
+
+ expect(mockHandler.handleResponse(mockBackendResponse)).andThrow(ioException);
+ HttpEntity streamingEntity = makeStreamingEntity();
+ expect(theResponse.getEntity()).andReturn(streamingEntity).anyTimes();
+
+ replayMocks();
+ Object result = null;
+ try {
+ result = impl.execute(host, request, mockHandler, context);
+ } catch (Exception e) {
+ assertEquals(ioException, e);
+ }
+ verifyMocks();
+ Assert.assertEquals(1, c.getCount());
+ Assert.assertNull(result);
+ assertTrue(cis.wasClosed());
}
@Test
@@ -769,15 +822,16 @@ public class TestCachingHttpClient {
return theResponse;
}
};
-
expect(mockHandler.handleResponse(mockBackendResponse)).andReturn(
theValue);
-
+ HttpEntity streamingEntity = makeStreamingEntity();
+ expect(theResponse.getEntity()).andReturn(streamingEntity);
replayMocks();
Object result = impl.execute(mockUriRequest, mockHandler, context);
verifyMocks();
Assert.assertEquals(1, c.getCount());
Assert.assertSame(theValue, result);
+ Assert.assertTrue(cis.wasClosed());
}
@Test
@@ -1942,6 +1996,13 @@ public class TestCachingHttpClient {
(HttpRequest)anyObject())).andThrow(exception);
}
+ private HttpEntity makeStreamingEntity() {
+ byte[] body = HttpTestUtils.getRandomBytes(101);
+ ByteArrayInputStream buf = new ByteArrayInputStream(body);
+ cis = new ConsumableInputStream(buf);
+ return new InputStreamEntity(cis, 101);
+ }
+
private void mockImplMethods(String... methods) {
mockedImpl = true;
impl = createMockBuilder(CachingHttpClient.class).withConstructor(
Modified: httpcomponents/httpclient/trunk/httpclient/src/main/java/org/apache/http/impl/client/AbstractHttpClient.java
URL: http://svn.apache.org/viewvc/httpcomponents/httpclient/trunk/httpclient/src/main/java/org/apache/http/impl/client/AbstractHttpClient.java?rev=1222845&r1=1222844&r2=1222845&view=diff
==============================================================================
--- httpcomponents/httpclient/trunk/httpclient/src/main/java/org/apache/http/impl/client/AbstractHttpClient.java (original)
+++ httpcomponents/httpclient/trunk/httpclient/src/main/java/org/apache/http/impl/client/AbstractHttpClient.java Fri Dec 23 20:59:54 2011
@@ -1079,7 +1079,7 @@ public abstract class AbstractHttpClient
T result;
try {
result = responseHandler.handleResponse(response);
- } catch (Throwable t) {
+ } catch (Exception t) {
HttpEntity entity = response.getEntity();
try {
EntityUtils.consume(entity);
@@ -1088,19 +1088,12 @@ public abstract class AbstractHttpClient
// important and will be thrown to the caller.
this.log.warn("Error consuming content after an exception.", t2);
}
-
- if (t instanceof Error) {
- throw (Error) t;
- }
-
if (t instanceof RuntimeException) {
throw (RuntimeException) t;
}
-
if (t instanceof IOException) {
throw (IOException) t;
}
-
throw new UndeclaredThrowableException(t);
}