diff --git a/httpclient-cache/src/main/java/org/apache/http/impl/client/cache/CachingExec.java b/httpclient-cache/src/main/java/org/apache/http/impl/client/cache/CachingExec.java index 72b837612..f0fc3f6d4 100644 --- a/httpclient-cache/src/main/java/org/apache/http/impl/client/cache/CachingExec.java +++ b/httpclient-cache/src/main/java/org/apache/http/impl/client/cache/CachingExec.java @@ -279,8 +279,7 @@ public class CachingExec implements ClientExecChain { } else if (!mayCallBackend(request)) { log.debug("Cache entry not suitable but only-if-cached requested"); out = Proxies.enhanceResponse(generateGatewayTimeout(context)); - } else if (validityPolicy.isRevalidatable(entry) - && !(entry.getStatusCode() == HttpStatus.SC_NOT_MODIFIED + } else if (!(entry.getStatusCode() == HttpStatus.SC_NOT_MODIFIED && !suitabilityChecker.isConditional(request))) { log.debug("Revalidating cache entry"); return revalidateCacheEntry(route, request, context, execAware, entry, now); diff --git a/httpclient-cache/src/main/java/org/apache/http/impl/client/cache/CachingHttpClient.java b/httpclient-cache/src/main/java/org/apache/http/impl/client/cache/CachingHttpClient.java index 9302bf97e..9a068846f 100644 --- a/httpclient-cache/src/main/java/org/apache/http/impl/client/cache/CachingHttpClient.java +++ b/httpclient-cache/src/main/java/org/apache/http/impl/client/cache/CachingHttpClient.java @@ -467,12 +467,9 @@ public class CachingHttpClient implements HttpClient { } else if (!mayCallBackend(request)) { log.debug("Cache entry not suitable but only-if-cached requested"); out = generateGatewayTimeout(context); - } else if (validityPolicy.isRevalidatable(entry)) { + } else { log.debug("Revalidating cache entry"); return revalidateCacheEntry(target, request, context, entry, now); - } else { - log.debug("Cache entry not usable; calling backend"); - return callBackend(target, request, context); } if (context != null) { context.setAttribute(ExecutionContext.HTTP_TARGET_HOST, target); diff --git a/httpclient-cache/src/test/java/org/apache/http/impl/client/cache/TestCachingExec.java b/httpclient-cache/src/test/java/org/apache/http/impl/client/cache/TestCachingExec.java index a29d52d53..10c392c25 100644 --- a/httpclient-cache/src/test/java/org/apache/http/impl/client/cache/TestCachingExec.java +++ b/httpclient-cache/src/test/java/org/apache/http/impl/client/cache/TestCachingExec.java @@ -56,6 +56,7 @@ import org.apache.http.conn.routing.HttpRoute; 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.easymock.IExpectationSetters; import org.easymock.classextension.EasyMock; import org.junit.Assert; @@ -187,7 +188,12 @@ public class TestCachingExec extends TestCachingExecChain { getCacheEntryReturns(mockCacheEntry); cacheEntrySuitable(false); cacheEntryValidatable(false); - implExpectsAnyRequestAndReturn(mockBackendResponse); + expect(mockConditionalRequestBuilder.buildConditionalRequest(request, mockCacheEntry)) + .andReturn(request); + backendExpectsRequestAndReturn(request, mockBackendResponse); + expect(mockBackendResponse.getProtocolVersion()).andReturn(HttpVersion.HTTP_1_1); + expect(mockBackendResponse.getStatusLine()).andReturn( + new BasicStatusLine(HttpVersion.HTTP_1_1, 200, "Ok")); replayMocks(); final HttpResponse result = impl.execute(route, request, context); @@ -196,7 +202,7 @@ public class TestCachingExec extends TestCachingExecChain { Assert.assertSame(mockBackendResponse, result); Assert.assertEquals(0, impl.getCacheMisses()); Assert.assertEquals(1, impl.getCacheHits()); - Assert.assertEquals(0, impl.getCacheUpdates()); + Assert.assertEquals(1, impl.getCacheUpdates()); } @Test diff --git a/httpclient-cache/src/test/java/org/apache/http/impl/client/cache/TestCachingExecChain.java b/httpclient-cache/src/test/java/org/apache/http/impl/client/cache/TestCachingExecChain.java index 010b0e791..e91997d3d 100644 --- a/httpclient-cache/src/test/java/org/apache/http/impl/client/cache/TestCachingExecChain.java +++ b/httpclient-cache/src/test/java/org/apache/http/impl/client/cache/TestCachingExecChain.java @@ -1650,7 +1650,7 @@ public abstract class TestCachingExecChain { protected void cacheEntryValidatable(final boolean b) { expect(mockValidityPolicy.isRevalidatable( - (HttpCacheEntry)anyObject())).andReturn(b); + (HttpCacheEntry)anyObject())).andReturn(b).anyTimes(); } protected void cacheEntryMustRevalidate(final boolean b) { diff --git a/httpclient-cache/src/test/java/org/apache/http/impl/client/cache/TestRFC5861Compliance.java b/httpclient-cache/src/test/java/org/apache/http/impl/client/cache/TestRFC5861Compliance.java index bf82be0fc..0458f4807 100644 --- a/httpclient-cache/src/test/java/org/apache/http/impl/client/cache/TestRFC5861Compliance.java +++ b/httpclient-cache/src/test/java/org/apache/http/impl/client/cache/TestRFC5861Compliance.java @@ -239,6 +239,31 @@ public class TestRFC5861Compliance extends AbstractProtocolTest { HttpTestUtils.assert110WarningFound(result); } + @Test + public void testStaleIfErrorInRequestIsTrueReturnsStaleNonRevalidatableEntryWithWarning() + throws Exception { + final Date tenSecondsAgo = new Date(new Date().getTime() - 10 * 1000L); + final HttpRequestWrapper req1 = HttpRequestWrapper.wrap(HttpTestUtils.makeDefaultRequest()); + final HttpResponse resp1 = HttpTestUtils.make200Response(); + resp1.setHeader("Date", DateUtils.formatDate(tenSecondsAgo)); + resp1.setHeader("Cache-Control", "public, max-age=5"); + + backendExpectsAnyRequestAndReturn(resp1); + + final HttpRequestWrapper req2 = HttpRequestWrapper.wrap(HttpTestUtils.makeDefaultRequest()); + req2.setHeader("Cache-Control", "public, stale-if-error=60"); + final HttpResponse resp2 = HttpTestUtils.make500Response(); + + backendExpectsAnyRequestAndReturn(resp2); + + replayMocks(); + impl.execute(route, req1, context, null); + final HttpResponse result = impl.execute(route, req2, context, null); + verifyMocks(); + + HttpTestUtils.assert110WarningFound(result); + } + @Test public void testStaleIfErrorInResponseIsFalseReturnsError() throws Exception{ @@ -341,6 +366,45 @@ public class TestRFC5861Compliance extends AbstractProtocolTest { assertTrue(warning110Found); } + @Test + public void testStaleWhileRevalidateReturnsStaleNonRevalidatableEntryWithWarning() + throws Exception { + config = CacheConfig.custom().setMaxCacheEntries(MAX_ENTRIES).setMaxObjectSize(MAX_BYTES) + .setAsynchronousWorkersMax(1).build(); + + impl = new CachingExec(mockBackend, cache, config, new AsynchronousValidator(config)); + + final HttpRequestWrapper req1 = HttpRequestWrapper.wrap(new BasicHttpRequest("GET", "/", + HttpVersion.HTTP_1_1)); + final HttpResponse resp1 = HttpTestUtils.make200Response(); + final Date now = new Date(); + final Date tenSecondsAgo = new Date(now.getTime() - 10 * 1000L); + resp1.setHeader("Cache-Control", "public, max-age=5, stale-while-revalidate=15"); + resp1.setHeader("Date", DateUtils.formatDate(tenSecondsAgo)); + + backendExpectsAnyRequestAndReturn(resp1).times(1, 2); + + final HttpRequestWrapper req2 = HttpRequestWrapper.wrap(new BasicHttpRequest("GET", "/", + HttpVersion.HTTP_1_1)); + + replayMocks(); + impl.execute(route, req1, context, null); + final HttpResponse result = impl.execute(route, req2, context, null); + verifyMocks(); + + assertEquals(HttpStatus.SC_OK, result.getStatusLine().getStatusCode()); + boolean warning110Found = false; + for (final Header h : result.getHeaders("Warning")) { + for (final WarningValue wv : WarningValue.getWarningValues(h)) { + if (wv.getWarnCode() == 110) { + warning110Found = true; + break; + } + } + } + assertTrue(warning110Found); + } + @Test public void testCanAlsoServeStale304sWhileRevalidating() throws Exception {