hc-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From ol...@apache.org
Subject 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/
Date Fri, 23 Dec 2011 20:59:54 GMT
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);
         }
 



Mime
View raw message