Return-Path: X-Original-To: apmail-hc-commits-archive@www.apache.org Delivered-To: apmail-hc-commits-archive@www.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id C1DD074E1 for ; Fri, 23 Dec 2011 20:56:53 +0000 (UTC) Received: (qmail 73721 invoked by uid 500); 23 Dec 2011 20:56:53 -0000 Delivered-To: apmail-hc-commits-archive@hc.apache.org Received: (qmail 73690 invoked by uid 500); 23 Dec 2011 20:56:53 -0000 Mailing-List: contact commits-help@hc.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: "HttpComponents Project" Delivered-To: mailing list commits@hc.apache.org Received: (qmail 73683 invoked by uid 99); 23 Dec 2011 20:56:53 -0000 Received: from nike.apache.org (HELO nike.apache.org) (192.87.106.230) by apache.org (qpsmtpd/0.29) with ESMTP; Fri, 23 Dec 2011 20:56:53 +0000 X-ASF-Spam-Status: No, hits=-2000.0 required=5.0 tests=ALL_TRUSTED X-Spam-Check-By: apache.org Received: from [140.211.11.4] (HELO eris.apache.org) (140.211.11.4) by apache.org (qpsmtpd/0.29) with ESMTP; Fri, 23 Dec 2011 20:56:50 +0000 Received: from eris.apache.org (localhost [127.0.0.1]) by eris.apache.org (Postfix) with ESMTP id 5410A23889ED for ; Fri, 23 Dec 2011 20:56:28 +0000 (UTC) Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Subject: svn commit: r1222840 - in /httpcomponents/httpclient/branches/4.1.x: ./ 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/cl... Date: Fri, 23 Dec 2011 20:56:28 -0000 To: commits@hc.apache.org From: olegk@apache.org X-Mailer: svnmailer-1.0.8-patched Message-Id: <20111223205628.5410A23889ED@eris.apache.org> X-Virus-Checked: Checked by ClamAV on apache.org Author: olegk Date: Fri Dec 23 20:56:27 2011 New Revision: 1222840 URL: http://svn.apache.org/viewvc?rev=1222840&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 Modified: httpcomponents/httpclient/branches/4.1.x/RELEASE_NOTES.txt httpcomponents/httpclient/branches/4.1.x/httpclient-cache/src/main/java/org/apache/http/impl/client/cache/CachingHttpClient.java httpcomponents/httpclient/branches/4.1.x/httpclient-cache/src/test/java/org/apache/http/impl/client/cache/TestCachingHttpClient.java httpcomponents/httpclient/branches/4.1.x/httpclient/src/main/java/org/apache/http/impl/client/AbstractHttpClient.java Modified: httpcomponents/httpclient/branches/4.1.x/RELEASE_NOTES.txt URL: http://svn.apache.org/viewvc/httpcomponents/httpclient/branches/4.1.x/RELEASE_NOTES.txt?rev=1222840&r1=1222839&r2=1222840&view=diff ============================================================================== --- httpcomponents/httpclient/branches/4.1.x/RELEASE_NOTES.txt (original) +++ httpcomponents/httpclient/branches/4.1.x/RELEASE_NOTES.txt Fri Dec 23 20:56:27 2011 @@ -1,6 +1,10 @@ Changes since 4.1.2 ------------------- +* [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 + * [HTTPCLIENT-1147] When HttpClient-Cache cannot open cache file, should act like miss. Contributed by Joe Campbell Modified: httpcomponents/httpclient/branches/4.1.x/httpclient-cache/src/main/java/org/apache/http/impl/client/cache/CachingHttpClient.java URL: http://svn.apache.org/viewvc/httpcomponents/httpclient/branches/4.1.x/httpclient-cache/src/main/java/org/apache/http/impl/client/cache/CachingHttpClient.java?rev=1222840&r1=1222839&r2=1222840&view=diff ============================================================================== --- httpcomponents/httpclient/branches/4.1.x/httpclient-cache/src/main/java/org/apache/http/impl/client/cache/CachingHttpClient.java (original) +++ httpcomponents/httpclient/branches/4.1.x/httpclient-cache/src/main/java/org/apache/http/impl/client/cache/CachingHttpClient.java Fri Dec 23 20:56:27 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.List; @@ -333,7 +334,7 @@ public class CachingHttpClient implement public T execute(HttpHost target, HttpRequest request, ResponseHandler 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 { @@ -355,7 +356,38 @@ public class CachingHttpClient implement public T execute(HttpUriRequest request, ResponseHandler responseHandler, HttpContext context) throws IOException { HttpResponse resp = execute(request, context); - return responseHandler.handleResponse(resp); + return handleAndConsume(responseHandler, resp); + } + + private T handleAndConsume( + final ResponseHandler 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/branches/4.1.x/httpclient-cache/src/test/java/org/apache/http/impl/client/cache/TestCachingHttpClient.java URL: http://svn.apache.org/viewvc/httpcomponents/httpclient/branches/4.1.x/httpclient-cache/src/test/java/org/apache/http/impl/client/cache/TestCachingHttpClient.java?rev=1222840&r1=1222839&r2=1222840&view=diff ============================================================================== --- httpcomponents/httpclient/branches/4.1.x/httpclient-cache/src/test/java/org/apache/http/impl/client/cache/TestCachingHttpClient.java (original) +++ httpcomponents/httpclient/branches/4.1.x/httpclient-cache/src/test/java/org/apache/http/impl/client/cache/TestCachingHttpClient.java Fri Dec 23 20:56:27 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/branches/4.1.x/httpclient/src/main/java/org/apache/http/impl/client/AbstractHttpClient.java URL: http://svn.apache.org/viewvc/httpcomponents/httpclient/branches/4.1.x/httpclient/src/main/java/org/apache/http/impl/client/AbstractHttpClient.java?rev=1222840&r1=1222839&r2=1222840&view=diff ============================================================================== --- httpcomponents/httpclient/branches/4.1.x/httpclient/src/main/java/org/apache/http/impl/client/AbstractHttpClient.java (original) +++ httpcomponents/httpclient/branches/4.1.x/httpclient/src/main/java/org/apache/http/impl/client/AbstractHttpClient.java Fri Dec 23 20:56:27 2011 @@ -943,7 +943,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); @@ -952,19 +952,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); }