From ee175e075703899c86f3f6999b47f515abb0019f Mon Sep 17 00:00:00 2001 From: Arturo Bernal Date: Wed, 19 Apr 2023 23:59:15 +0200 Subject: [PATCH] Handling for 304 Not Modified responses in CachingHttpAsyncClient and c. When a 304 response is received, the cache entry is updated and the updated entry is used to generate the response. --- .../http/impl/cache/AsyncCachingExec.java | 61 +++++++++++++++++ .../client5/http/impl/cache/CachingExec.java | 56 ++++++++++++++-- .../http/impl/cache/TestCachingExecChain.java | 65 ++++++++++++++++++- 3 files changed, 177 insertions(+), 5 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 379f4b6d6..d41a27752 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 @@ -1010,6 +1010,67 @@ public AsyncDataConsumer handleResponse( backendResponse.addHeader("Via", generateViaHeader(backendResponse)); final AsyncExecCallback callback; + // Handle 304 Not Modified responses + if (backendResponse.getCode() == HttpStatus.SC_NOT_MODIFIED) { + responseCache.getCacheEntry(target, request, new FutureCallback() { + @Override + public void completed(final HttpCacheEntry existingEntry) { + if (existingEntry != null) { + if (LOG.isDebugEnabled()) { + LOG.debug("Existing cache entry found, updating cache entry"); + } + responseCache.updateCacheEntry( + target, + request, + existingEntry, + backendResponse, + requestDate, + responseDate, + new FutureCallback() { + + @Override + public void completed(final HttpCacheEntry updatedEntry) { + try { + if (LOG.isDebugEnabled()) { + LOG.debug("Cache entry updated, generating response from updated entry"); + } + final SimpleHttpResponse cacheResponse = responseGenerator.generateResponse(request, updatedEntry); + triggerResponse(cacheResponse, scope, asyncExecCallback); + } catch (final ResourceIOException ex) { + asyncExecCallback.failed(ex); + } + } + @Override + public void failed(final Exception cause) { + if (LOG.isDebugEnabled()) { + LOG.debug("Request failed: {}", cause.getMessage()); + } + asyncExecCallback.failed(cause); + } + + @Override + public void cancelled() { + if (LOG.isDebugEnabled()) { + LOG.debug("Cache entry updated aborted"); + } + asyncExecCallback.failed(new InterruptedIOException()); + } + + }); + } + } + + @Override + public void failed(final Exception cause) { + asyncExecCallback.failed(cause); + } + + @Override + public void cancelled() { + asyncExecCallback.failed(new InterruptedIOException()); + } + }); + } if (backendResponse.getCode() != HttpStatus.SC_NOT_MODIFIED) { callback = new BackendResponseHandler(target, request, requestDate, responseDate, scope, asyncExecCallback); 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 e03196938..9510a9a26 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 @@ -103,17 +103,24 @@ class CachingExec extends CachingExecBase implements ExecChainHandler { private final HttpCache responseCache; private final DefaultCacheRevalidator cacheRevalidator; private final ConditionalRequestBuilder conditionalRequestBuilder; + private final CacheUpdateHandler cacheUpdateHandler; private static final Logger LOG = LoggerFactory.getLogger(CachingExec.class); - CachingExec(final HttpCache cache, final DefaultCacheRevalidator cacheRevalidator, final CacheConfig config) { + CachingExec(final HttpCache cache, final DefaultCacheRevalidator cacheRevalidator, final CacheConfig config, final CacheUpdateHandler cacheUpdateHandler) { super(config); this.responseCache = Args.notNull(cache, "Response cache"); this.cacheRevalidator = cacheRevalidator; this.conditionalRequestBuilder = new ConditionalRequestBuilder<>(classicHttpRequest -> - ClassicRequestBuilder.copy(classicHttpRequest).build()); + ClassicRequestBuilder.copy(classicHttpRequest).build()); + this.cacheUpdateHandler = cacheUpdateHandler != null ? cacheUpdateHandler: new CacheUpdateHandler(); + } + + CachingExec(final HttpCache cache, final DefaultCacheRevalidator cacheRevalidator, final CacheConfig config) { + this(cache, cacheRevalidator, config, null); } + CachingExec( final HttpCache responseCache, final CacheValidityPolicy validityPolicy, @@ -125,12 +132,37 @@ class CachingExec extends CachingExecBase implements ExecChainHandler { final RequestProtocolCompliance requestCompliance, final DefaultCacheRevalidator cacheRevalidator, final ConditionalRequestBuilder conditionalRequestBuilder, - final CacheConfig config) { + final CacheConfig config, + final CacheUpdateHandler cacheUpdateHandler) { super(validityPolicy, responseCachingPolicy, responseGenerator, cacheableRequestPolicy, suitabilityChecker, responseCompliance, requestCompliance, config); this.responseCache = responseCache; this.cacheRevalidator = cacheRevalidator; this.conditionalRequestBuilder = conditionalRequestBuilder; + this.cacheUpdateHandler = cacheUpdateHandler; + } + + + CachingExec( + final HttpCache responseCache, + final CacheValidityPolicy validityPolicy, + final ResponseCachingPolicy responseCachingPolicy, + final CachedHttpResponseGenerator responseGenerator, + final CacheableRequestPolicy cacheableRequestPolicy, + final CachedResponseSuitabilityChecker suitabilityChecker, + final ResponseProtocolCompliance responseCompliance, + final RequestProtocolCompliance requestCompliance, + final DefaultCacheRevalidator cacheRevalidator, + final ConditionalRequestBuilder conditionalRequestBuilder, + final CacheConfig config) { + this(responseCache,validityPolicy,responseCachingPolicy,responseGenerator,cacheableRequestPolicy, + suitabilityChecker, + responseCompliance, + requestCompliance, + cacheRevalidator, + conditionalRequestBuilder, + config, + null); } CachingExec( @@ -140,7 +172,7 @@ class CachingExec extends CachingExecBase implements ExecChainHandler { final CacheConfig config) { this(cache, executorService != null ? new DefaultCacheRevalidator(executorService, schedulingStrategy) : null, - config); + config, null); } CachingExec( @@ -395,6 +427,22 @@ ClassicHttpResponse cacheAndReturnResponse( final Instant requestSent, final Instant responseReceived) throws IOException { LOG.debug("Caching backend response"); + + // handle 304 Not Modified responses + if (backendResponse.getCode() == HttpStatus.SC_NOT_MODIFIED) { + final HttpCacheEntry existingEntry = responseCache.getCacheEntry(target, request); + if (existingEntry != null) { + final HttpCacheEntry updatedEntry = cacheUpdateHandler.updateCacheEntry( + request.getMethod(), + existingEntry, + requestSent, + responseReceived, + backendResponse); + + return convert(responseGenerator.generateResponse(request, updatedEntry), scope); + } + } + final ByteArrayBuffer buf; final HttpEntity entity = backendResponse.getEntity(); if (entity != null) { 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 d8bba3720..2b5296ca9 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 @@ -34,12 +34,15 @@ import java.io.InputStream; import java.net.SocketException; import java.net.SocketTimeoutException; +import java.nio.charset.StandardCharsets; 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 java.util.HashMap; +import java.util.Map; import org.apache.hc.client5.http.HttpRoute; import org.apache.hc.client5.http.async.methods.SimpleHttpResponse; @@ -68,6 +71,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.http.message.BasicHeader; import org.apache.hc.core5.net.URIAuthority; import org.apache.hc.core5.util.TimeValue; import org.junit.jupiter.api.Assertions; @@ -91,6 +95,9 @@ public class TestCachingExecChain { private CachedHttpResponseGenerator responseGenerator; @Spy HttpCache cache = new BasicHttpCache(); + @Mock + CacheUpdateHandler cacheUpdateHandler; + CacheConfig config; HttpRoute route; HttpHost host; @@ -130,7 +137,7 @@ public void setUp() { conditionalRequestBuilder = mock(ConditionalRequestBuilder.class); responseCache = mock(HttpCache.class); - impl = new CachingExec(cache, null, CacheConfig.DEFAULT); + impl = new CachingExec(cache, null, CacheConfig.DEFAULT, cacheUpdateHandler); } public ClassicHttpResponse execute(final ClassicHttpRequest request) throws IOException, HttpException { @@ -1413,6 +1420,62 @@ public void testReturnssetStaleIfErrorEnabled() throws Exception { Mockito.verify(cacheRevalidator, Mockito.times(1)).revalidateCacheEntry(Mockito.any(), Mockito.any()); } + @Test + public void testNotModifiedResponseUpdatesCacheEntry() throws Exception { + // Prepare request and host + final HttpHost host = new HttpHost("foo.example.com"); + final ClassicHttpRequest request = new HttpGet("http://foo.example.com/bar"); + + // Prepare original cache entry + final HttpCacheEntry originalEntry = HttpTestUtils.makeCacheEntry(); + Mockito.when(cache.getCacheEntry(host, request)).thenReturn(originalEntry); + + // Prepare 304 Not Modified response + final Instant now = Instant.now(); + final Instant requestSent = now.plusSeconds(3); + final Instant responseReceived = now.plusSeconds(1); + + final ClassicHttpResponse backendResponse = new BasicClassicHttpResponse(HttpStatus.SC_NOT_MODIFIED, "Not Modified"); + backendResponse.setHeader("Cache-Control", "public, max-age=3600"); + backendResponse.setHeader("ETag", "\"etag\""); + + final ExecChain.Scope scope = new ExecChain.Scope("test", route, request, mockExecRuntime, context); + + final Header[] headers = new Header[5]; + for (int i = 0; i < headers.length; i++) { + headers[i] = new BasicHeader("header" + i, "value" + i); + } + final String body = "Lorem ipsum dolor sit amet"; + + final Map variantMap = new HashMap<>(); + variantMap.put("test variant 1", "true"); + variantMap.put("test variant 2", "true"); + final HttpCacheEntry cacheEntry = new HttpCacheEntry( + Instant.now(), + Instant.now(), + HttpStatus.SC_NOT_MODIFIED, + headers, + new HeapResource(body.getBytes(StandardCharsets.UTF_8)), variantMap); + + Mockito.when(cacheUpdateHandler.updateCacheEntry(Mockito.any(), Mockito.any(), Mockito.any(), Mockito.any(), Mockito.any())).thenReturn(cacheEntry); + + // Call cacheAndReturnResponse with 304 Not Modified response + final ClassicHttpResponse cachedResponse = impl.cacheAndReturnResponse(host, request, backendResponse, scope, requestSent, responseReceived); + + + // Verify cache entry is updated + Mockito.verify(cacheUpdateHandler).updateCacheEntry( + request.getMethod(), + originalEntry, + requestSent, + responseReceived, + backendResponse + ); + + // Verify response is generated from the updated cache entry + Assertions.assertEquals(HttpStatus.SC_NOT_MODIFIED, cachedResponse.getCode()); + } + @Test public void testStaleIfErrorEnabledWithIOException() throws Exception {