HTTPCLIENT-1425: Fixed socket closed exception thrown by caching HttpClient when the origin server sends a long chunked response

Contributed by James Leigh <james at 3roundstones dot com>

git-svn-id: https://svn.apache.org/repos/asf/httpcomponents/httpclient/trunk@1534585 13f79535-47bb-0310-9956-ffa450edef68

RELEASE_NOTES.txt

@@ -1,6 +1,10 @@
 Changes since 4.3.1
 -------------------
 
+* [HTTPCLIENT-1425] Fixed socket closed exception thrown by caching HttpClient when the origin 
+  server sends a long chunked response.
+  Contributed by James Leigh <james at 3roundstones dot com>
+
 * [HTTPCLIENT-1417] Fixed NPE in BrowserCompatSpec#formatCookies caused by version 1
   cookies with null cookie value.
   Contributed by Oleg Kalnichevski <olegk at apache.org>

httpclient-cache/src/main/java/org/apache/http/impl/client/cache/BasicHttpCache.java

@@ -50,10 +50,10 @@ import org.apache.http.client.cache.HttpCacheUpdateCallback;
 import org.apache.http.client.cache.HttpCacheUpdateException;
 import org.apache.http.client.cache.Resource;
 import org.apache.http.client.cache.ResourceFactory;
+import org.apache.http.client.methods.CloseableHttpResponse;
 import org.apache.http.entity.ByteArrayEntity;
 import org.apache.http.message.BasicHttpResponse;
 import org.apache.http.protocol.HTTP;
-import org.apache.http.util.EntityUtils;
 
 class BasicHttpCache implements HttpCache {
     private static final Set<String> safeRequestMethods = new HashSet<String>(
@@ -276,12 +276,25 @@ class BasicHttpCache implements HttpCache {
     public HttpResponse cacheAndReturnResponse(final HttpHost host, final HttpRequest request,
             final HttpResponse originResponse, final Date requestSent, final Date responseReceived)
             throws IOException {
+        return cacheAndReturnResponse(host, request,
+                Proxies.enhanceResponse(originResponse), requestSent,
+                responseReceived);
+    }
 
+    public HttpResponse cacheAndReturnResponse(
+            final HttpHost host,
+            final HttpRequest request,
+            final CloseableHttpResponse originResponse,
+            final Date requestSent,
+            final Date responseReceived) throws IOException {
+
+        boolean closeOriginResponse = true;
         final SizeLimitedResponseReader responseReader = getResponseReader(request, originResponse);
         try {
             responseReader.readResponse();
 
             if (responseReader.isLimitReached()) {
+                closeOriginResponse = false;
                 return responseReader.getReconstructedResponse();
             }
 
@@ -298,12 +311,10 @@ class BasicHttpCache implements HttpCache {
                     resource);
             storeInCache(host, request, entry);
             return responseGenerator.generateResponse(entry);
-        } catch (final IOException ex) {
+        } finally {
-            EntityUtils.consume(originResponse.getEntity());
+            if (closeOriginResponse) {
-            throw ex;
+                originResponse.close();
-        } catch (final RuntimeException ex) {
+            }
-            EntityUtils.consumeQuietly(originResponse.getEntity());
-            throw ex;
         }
     }
 

httpclient-cache/src/main/java/org/apache/http/impl/client/cache/CachingExec.java

@@ -808,13 +808,9 @@ public class CachingExec implements ClientExecChain {
         final boolean cacheable = responseCachingPolicy.isResponseCacheable(request, backendResponse);
         responseCache.flushInvalidatedCacheEntriesFor(target, request, backendResponse);
         if (cacheable && !alreadyHaveNewerCacheEntry(target, request, backendResponse)) {
-            try {
             storeRequestIfModifiedSinceFor304Response(request, backendResponse);
             return Proxies.enhanceResponse(responseCache.cacheAndReturnResponse(
                     target, request, backendResponse, requestDate, responseDate));
-            } finally {
-                backendResponse.close();
-            }
         }
         if (!cacheable) {
             try {

httpclient-cache/src/main/java/org/apache/http/impl/client/cache/HttpCache.java

@@ -34,6 +34,7 @@ import org.apache.http.HttpHost;
 import org.apache.http.HttpRequest;
 import org.apache.http.HttpResponse;
 import org.apache.http.client.cache.HttpCacheEntry;
+import org.apache.http.client.methods.CloseableHttpResponse;
 
 /**
  * @since 4.1
@@ -103,6 +104,21 @@ interface HttpCache {
             Date requestSent, Date responseReceived)
         throws IOException;
 
+    /**
+     * Store a {@link HttpResponse} in the cache if possible, and return
+     * @param host
+     * @param request
+     * @param originResponse
+     * @param requestSent
+     * @param responseReceived
+     * @return the {@link HttpResponse}
+     * @throws IOException
+     */
+    HttpResponse cacheAndReturnResponse(HttpHost host, HttpRequest request,
+            CloseableHttpResponse originResponse, Date requestSent,
+            Date responseReceived)
+        throws IOException;
+
     /**
      * Update a {@link HttpCacheEntry} using a 304 {@link HttpResponse}.
      * @param target

httpclient-cache/src/test/java/org/apache/http/impl/client/cache/AbstractProtocolTest.java

@@ -72,6 +72,11 @@ public abstract class AbstractProtocolTest {
         return null;
     }
 
+    public static CloseableHttpResponse eqCloseableResponse(final CloseableHttpResponse in) {
+        EasyMock.reportMatcher(new ResponseEquivalent(in));
+        return null;
+    }
+
     @Before
     public void setUp() {
         host = new HttpHost("foo.example.com");

httpclient-cache/src/test/java/org/apache/http/impl/client/cache/TestCachingExec.java

@@ -36,11 +36,14 @@ import static org.easymock.classextension.EasyMock.createMockBuilder;
 import static org.easymock.classextension.EasyMock.createNiceMock;
 import static org.easymock.classextension.EasyMock.replay;
 import static org.easymock.classextension.EasyMock.verify;
+import static org.junit.Assert.assertTrue;
 
 import java.io.IOException;
+import java.io.InputStream;
 import java.util.Date;
 import java.util.HashMap;
 import java.util.Map;
+import java.util.concurrent.atomic.AtomicInteger;
 
 import org.apache.http.HttpHost;
 import org.apache.http.HttpRequest;
@@ -52,11 +55,14 @@ import org.apache.http.client.methods.CloseableHttpResponse;
 import org.apache.http.client.methods.HttpExecutionAware;
 import org.apache.http.client.methods.HttpRequestWrapper;
 import org.apache.http.client.protocol.HttpClientContext;
+import org.apache.http.client.utils.DateUtils;
 import org.apache.http.conn.routing.HttpRoute;
+import org.apache.http.entity.InputStreamEntity;
 import org.apache.http.impl.execchain.ClientExecChain;
 import org.apache.http.message.BasicHttpRequest;
 import org.apache.http.message.BasicHttpResponse;
 import org.apache.http.message.BasicStatusLine;
+import org.apache.http.protocol.HTTP;
 import org.easymock.IExpectationSetters;
 import org.easymock.classextension.EasyMock;
 import org.junit.Assert;
@@ -92,7 +98,7 @@ public class TestCachingExec extends TestCachingExecChain {
     }
 
     @Override
-    public ClientExecChain createCachingExecChain(final ClientExecChain mockBackend,
+    public CachingExec createCachingExecChain(final ClientExecChain mockBackend,
             final HttpCache mockCache, final CacheValidityPolicy mockValidityPolicy,
             final ResponseCachingPolicy mockResponsePolicy,
             final CachedHttpResponseGenerator mockResponseGenerator,
@@ -118,7 +124,7 @@ public class TestCachingExec extends TestCachingExecChain {
     }
 
     @Override
-    public ClientExecChain createCachingExecChain(final ClientExecChain backend,
+    public CachingExec createCachingExecChain(final ClientExecChain backend,
             final HttpCache cache, final CacheConfig config) {
         return impl = new CachingExec(backend, cache, config);
     }
@@ -300,6 +306,53 @@ public class TestCachingExec extends TestCachingExecChain {
         verifyMocks();
     }
 
+    @Test
+    public void testEndlessResponsesArePassedThrough() throws Exception {
+        impl = createCachingExecChain(mockBackend, new BasicHttpCache(), CacheConfig.DEFAULT);
+
+        final HttpResponse resp1 = new BasicHttpResponse(HttpVersion.HTTP_1_1, HttpStatus.SC_OK, "OK");
+        resp1.setHeader("Date", DateUtils.formatDate(new Date()));
+        resp1.setHeader("Server", "MockOrigin/1.0");
+        resp1.setHeader(HTTP.TRANSFER_ENCODING, HTTP.CHUNK_CODING);
+
+        final AtomicInteger size = new AtomicInteger();
+        final AtomicInteger maxlength = new AtomicInteger(Integer.MAX_VALUE);
+        resp1.setEntity(new InputStreamEntity(new InputStream() {
+            private Throwable closed;
+
+            public void close() throws IOException {
+                closed = new Throwable();
+                super.close();
+            }
+
+            public int read() throws IOException {
+                Thread.yield();
+                if (closed != null) {
+                    throw new IOException("Response has been closed");
+
+                }
+                if (size.incrementAndGet() > maxlength.get())
+                    return -1;
+                return 'y';
+            }
+        }));
+
+        final CloseableHttpResponse resp = mockBackend.execute(
+                EasyMock.isA(HttpRoute.class),
+                EasyMock.isA(HttpRequestWrapper.class),
+                EasyMock.isA(HttpClientContext.class),
+                EasyMock.<HttpExecutionAware>isNull());
+        EasyMock.expect(resp).andReturn(Proxies.enhanceResponse(resp1));
+
+        final HttpRequestWrapper req1 = HttpRequestWrapper.wrap(HttpTestUtils.makeDefaultRequest());
+
+        replayMocks();
+        final CloseableHttpResponse resp2 = impl.execute(route, req1, context, null);
+        maxlength.set(size.get() * 2);
+        verifyMocks();
+        assertTrue(HttpTestUtils.semanticallyTransparent(resp1, resp2));
+    }
+
     @Test
     public void testCallBackendMakesBackEndRequestAndHandlesResponse() throws Exception {
         mockImplMethods(GET_CURRENT_DATE, HANDLE_BACKEND_RESPONSE);

httpclient-cache/src/test/java/org/apache/http/impl/client/cache/TestCachingExecChain.java

@@ -1319,7 +1319,7 @@ public abstract class TestCachingExecChain {
                 isA(HttpRequest.class)))
             .andThrow(new IOException()).anyTimes();
         expect(mockCache.cacheAndReturnResponse(eq(host),
-                isA(HttpRequest.class), isA(HttpResponse.class),
+                isA(HttpRequest.class), isA(CloseableHttpResponse.class),
                 isA(Date.class), isA(Date.class)))
             .andReturn(resp).anyTimes();
         expect(mockBackend.execute(

httpclient-cache/src/test/java/org/apache/http/impl/client/cache/TestProtocolRequirements.java

@@ -2899,7 +2899,7 @@ public class TestProtocolRequirements extends AbstractProtocolTest {
         EasyMock.expect(mockCache.cacheAndReturnResponse(
                 EasyMock.isA(HttpHost.class),
                 EasyMock.isA(HttpRequestWrapper.class),
-                eqResponse(validated),
+                eqCloseableResponse(validated),
                 EasyMock.isA(Date.class),
                 EasyMock.isA(Date.class))).andReturn(reconstructed).times(0, 1);