From e1cfb2add6d3a1e5797cdfdcc0391d8e36d5dad8 Mon Sep 17 00:00:00 2001 From: Arturo Bernal Date: Fri, 21 Apr 2023 22:11:30 +0200 Subject: [PATCH] Complete the implementation of stale-if-error support as per RFC 5861 - Updated handling of IOExceptions to serve stale responses when allowed by stale-if-error --- .../http/impl/cache/AsyncCachingExec.java | 20 ++++- .../client5/http/impl/cache/CachingExec.java | 9 +- .../http/impl/cache/TestCachingExecChain.java | 86 +++++++++++++++++++ 3 files changed, 112 insertions(+), 3 deletions(-) diff --git a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/AsyncCachingExec.java b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/AsyncCachingExec.java index d22432612..0ca8b2f0b 100644 --- a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/AsyncCachingExec.java +++ b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/AsyncCachingExec.java @@ -648,10 +648,11 @@ class AsyncCachingExec extends CachingExecBase implements AsyncExecChainHandler triggerResponse(cacheResponse, scope, asyncExecCallback); } else if (!(entry.getStatus() == HttpStatus.SC_NOT_MODIFIED && !suitabilityChecker.isConditional(request))) { LOG.debug("Revalidating cache entry"); + final boolean staleIfErrorEnabled = responseCachingPolicy.isStaleIfErrorEnabled(entry); if (cacheRevalidator != null && !staleResponseNotAllowed(request, entry, now) && validityPolicy.mayReturnStaleWhileRevalidating(entry, now) - || responseCachingPolicy.isStaleIfErrorEnabled(entry)) { + || staleIfErrorEnabled) { LOG.debug("Serving stale with asynchronous revalidation"); try { final SimpleHttpResponse cacheResponse = generateCachedResponse(request, context, entry, now); @@ -672,7 +673,22 @@ class AsyncCachingExec extends CachingExecBase implements AsyncExecChainHandler asyncExecCallback1 -> revalidateCacheEntry(target, request, entityProducer, fork, chain, asyncExecCallback1, entry)); triggerResponse(cacheResponse, scope, asyncExecCallback); } catch (final ResourceIOException ex) { - asyncExecCallback.failed(ex); + if (staleIfErrorEnabled) { + if (LOG.isDebugEnabled()) { + LOG.debug("Serving stale response due to IOException and stale-if-error enabled"); + } + try { + final SimpleHttpResponse cacheResponse = generateCachedResponse(request, context, entry, now); + triggerResponse(cacheResponse, scope, asyncExecCallback); + } catch (final ResourceIOException ex2) { + if (LOG.isDebugEnabled()) { + LOG.debug("Failed to generate cached response, falling back to backend", ex2); + } + callBackend(target, request, entityProducer, scope, chain, asyncExecCallback); + } + } else { + asyncExecCallback.failed(ex); + } } } else { revalidateCacheEntry(target, request, entityProducer, scope, chain, asyncExecCallback, entry); diff --git a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachingExec.java b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachingExec.java index 7d7d392aa..fd649fdc4 100644 --- a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachingExec.java +++ b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachingExec.java @@ -271,11 +271,12 @@ class CachingExec extends CachingExecBase implements ExecChainHandler { return convert(generateGatewayTimeout(context), scope); } else if (!(entry.getStatus() == HttpStatus.SC_NOT_MODIFIED && !suitabilityChecker.isConditional(request))) { LOG.debug("Revalidating cache entry"); + final boolean staleIfErrorEnabled = responseCachingPolicy.isStaleIfErrorEnabled(entry); try { if (cacheRevalidator != null && !staleResponseNotAllowed(request, entry, now) && validityPolicy.mayReturnStaleWhileRevalidating(entry, now) - || responseCachingPolicy.isStaleIfErrorEnabled(entry)) { + || staleIfErrorEnabled) { LOG.debug("Serving stale with asynchronous revalidation"); final String exchangeId = ExecSupport.getNextExchangeId(); context.setExchangeId(exchangeId); @@ -293,6 +294,12 @@ class CachingExec extends CachingExecBase implements ExecChainHandler { } return revalidateCacheEntry(target, request, scope, chain, entry); } catch (final IOException ioex) { + if (staleIfErrorEnabled) { + if (LOG.isDebugEnabled()) { + LOG.debug("Serving stale response due to IOException and stale-if-error enabled"); + } + return convert(generateCachedResponse(request, context, entry, now), scope); + } return convert(handleRevalidationFailure(request, context, entry, now), scope); } } else { diff --git a/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestCachingExecChain.java b/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestCachingExecChain.java index f701e29a8..7f1d3801c 100644 --- a/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestCachingExecChain.java +++ b/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestCachingExecChain.java @@ -37,6 +37,9 @@ import java.net.SocketTimeoutException; import java.time.Duration; import java.time.Instant; import java.time.temporal.ChronoUnit; +import java.util.Arrays; +import java.util.Optional; +import java.util.concurrent.atomic.AtomicInteger; import org.apache.hc.client5.http.HttpRoute; import org.apache.hc.client5.http.async.methods.SimpleHttpResponse; @@ -45,6 +48,7 @@ import org.apache.hc.client5.http.cache.CacheResponseStatus; import org.apache.hc.client5.http.cache.HttpCacheContext; import org.apache.hc.client5.http.cache.HttpCacheEntry; import org.apache.hc.client5.http.cache.HttpCacheStorage; +import org.apache.hc.client5.http.cache.ResourceIOException; import org.apache.hc.client5.http.classic.ExecChain; import org.apache.hc.client5.http.classic.ExecRuntime; import org.apache.hc.client5.http.classic.methods.HttpGet; @@ -53,6 +57,7 @@ import org.apache.hc.client5.http.protocol.HttpClientContext; import org.apache.hc.client5.http.utils.DateUtils; import org.apache.hc.core5.http.ClassicHttpRequest; import org.apache.hc.core5.http.ClassicHttpResponse; +import org.apache.hc.core5.http.Header; import org.apache.hc.core5.http.HttpException; import org.apache.hc.core5.http.HttpHost; import org.apache.hc.core5.http.HttpRequest; @@ -64,6 +69,7 @@ import org.apache.hc.core5.http.io.support.ClassicRequestBuilder; import org.apache.hc.core5.http.message.BasicClassicHttpRequest; import org.apache.hc.core5.http.message.BasicClassicHttpResponse; import org.apache.hc.core5.net.URIAuthority; +import org.apache.hc.core5.util.TimeValue; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -82,6 +88,7 @@ public class TestCachingExecChain { @Mock HttpCacheStorage mockStorage; private DefaultCacheRevalidator cacheRevalidator; + private CachedHttpResponseGenerator responseGenerator; @Spy HttpCache cache = new BasicHttpCache(); CacheConfig config; @@ -91,6 +98,15 @@ public class TestCachingExecChain { HttpCacheContext context; HttpCacheEntry entry; CachingExec impl; + CacheValidityPolicy validityPolicy; + ResponseCachingPolicy responseCachingPolicy; + CacheableRequestPolicy cacheableRequestPolicy; + CachedResponseSuitabilityChecker suitabilityChecker; + ResponseProtocolCompliance responseCompliance; + RequestProtocolCompliance requestCompliance; + ConditionalRequestBuilder conditionalRequestBuilder; + CacheConfig customConfig; + HttpCache responseCache; @BeforeEach public void setUp() { @@ -102,6 +118,17 @@ public class TestCachingExecChain { context = HttpCacheContext.create(); entry = HttpTestUtils.makeCacheEntry(); cacheRevalidator = mock(DefaultCacheRevalidator.class); + responseGenerator = mock(CachedHttpResponseGenerator.class); + customConfig = mock(CacheConfig.class); + + validityPolicy = mock(CacheValidityPolicy.class); + responseCachingPolicy = mock(ResponseCachingPolicy.class); + cacheableRequestPolicy = mock(CacheableRequestPolicy.class); + suitabilityChecker = mock(CachedResponseSuitabilityChecker.class); + responseCompliance = mock(ResponseProtocolCompliance.class); + requestCompliance = mock(RequestProtocolCompliance.class); + conditionalRequestBuilder = mock(ConditionalRequestBuilder.class); + responseCache = mock(HttpCache.class); impl = new CachingExec(cache, null, CacheConfig.DEFAULT); } @@ -1359,4 +1386,63 @@ public class TestCachingExecChain { Mockito.verify(cacheRevalidator, Mockito.times(1)).revalidateCacheEntry(Mockito.any(), Mockito.any()); } + @Test + public void testStaleIfErrorEnabledWithIOException() throws Exception { + + impl = new CachingExec(responseCache, validityPolicy, responseCachingPolicy, + responseGenerator, cacheableRequestPolicy, suitabilityChecker, + responseCompliance, requestCompliance, cacheRevalidator, + conditionalRequestBuilder, customConfig); + // Create the first request and response + final BasicClassicHttpRequest req1 = new BasicClassicHttpRequest("GET", "http://foo.example.com/"); + final BasicClassicHttpRequest req2 = new BasicClassicHttpRequest("GET", "http://foo.example.com/"); + final ClassicHttpResponse resp1 = new BasicClassicHttpResponse(HttpStatus.SC_GATEWAY_TIMEOUT, "OK"); + resp1.setEntity(HttpTestUtils.makeBody(128)); + resp1.setHeader("Content-Length", "128"); + resp1.setHeader("ETag", "\"etag\""); + resp1.setHeader("Date", DateUtils.formatStandardDate(Instant.now().minus(Duration.ofHours(10)))); + resp1.setHeader("Cache-Control", "public, max-age=-1, stale-while-revalidate=1"); + final ClassicHttpResponse resp2 = HttpTestUtils.make200Response(); + + // Set up the mock response chain + Mockito.when(mockExecChain.proceed(Mockito.any(), Mockito.any())).thenReturn(resp1); + Mockito.when(responseCachingPolicy.isStaleIfErrorEnabled(Mockito.any())).thenReturn(true); + Mockito.when(cacheableRequestPolicy.isServableFromCache(Mockito.any())).thenReturn(true); + Mockito.when(validityPolicy.getStaleness(Mockito.any(), Mockito.any())).thenReturn(TimeValue.MAX_VALUE); + + final SimpleHttpResponse response = SimpleHttpResponse.create(HttpStatus.SC_OK); + final AtomicInteger callCount = new AtomicInteger(0); + + Mockito.doAnswer(invocation -> { + if (callCount.getAndIncrement() == 0) { + throw new ResourceIOException("ResourceIOException"); + } else { + // Replace this with the actual return value for the second call + return response; + } + }).when(responseGenerator).generateResponse(Mockito.any(), Mockito.any()); + + // Execute the first request and assert the response + final ClassicHttpResponse response1 = execute(req1); + final HttpCacheEntry httpCacheEntry = HttpTestUtils.makeCacheEntry(); + Mockito.when(responseCache.getCacheEntry(Mockito.any(), Mockito.any())).thenReturn(httpCacheEntry); + Assertions.assertEquals(HttpStatus.SC_GATEWAY_TIMEOUT, response1.getCode()); + + Mockito.when(mockExecRuntime.fork(Mockito.any())).thenReturn(mockExecRuntime); + Mockito.when(mockExecChain.proceed(Mockito.any(), Mockito.any())).thenReturn(resp2); + + final ClassicHttpResponse response2 = execute(req2); + // Verify that the response has the expected status code (e.g., 200) + Assertions.assertEquals(HttpStatus.SC_OK, response2.getCode()); + + // Verify that the response has the expected headers or other properties + // For example, you can check for the "Warning" header indicating a stale response: + final Optional
warningHeader = Arrays.stream(response.getHeaders()) + .filter(header -> header.getName().equalsIgnoreCase("Warning")) + .findFirst(); + + Assertions.assertTrue(warningHeader.isPresent()); + Assertions.assertTrue(warningHeader.get().getValue().contains("110")); + } + }