From 4cdbd3df31b7f6a99f0368185806920cecb16ae7 Mon Sep 17 00:00:00 2001 From: Oleg Kalnichevski Date: Mon, 30 Oct 2023 17:29:20 +0100 Subject: [PATCH 1/8] HTTPCLIENT-2277: Cache update bug fix --- .../http/cache/HttpCacheEntryFactory.java | 13 ++++++++--- .../http/impl/cache/BasicHttpAsyncCache.java | 4 ++++ .../http/impl/cache/BasicHttpCache.java | 6 ++++- .../http/cache/TestHttpCacheEntryFactory.java | 22 ++++++++++++------- 4 files changed, 33 insertions(+), 12 deletions(-) diff --git a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/cache/HttpCacheEntryFactory.java b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/cache/HttpCacheEntryFactory.java index f8dc90d816..91c6ae26ad 100644 --- a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/cache/HttpCacheEntryFactory.java +++ b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/cache/HttpCacheEntryFactory.java @@ -232,12 +232,16 @@ public HttpCacheEntry create(final Instant requestInstant, * * @param requestInstant Date/time when the request was made (Used for age calculations) * @param responseInstant Date/time that the response came back (Used for age calculations) + * @param host Target host + * @param request Original client request (a deep copy of this object is made) * @param response Origin response (a deep copy of this object is made) * @param entry Existing cache entry. */ public HttpCacheEntry createUpdated( final Instant requestInstant, final Instant responseInstant, + final HttpHost host, + final HttpRequest request, final HttpResponse response, final HttpCacheEntry entry) { Args.notNull(requestInstant, "Request instant"); @@ -249,13 +253,16 @@ public HttpCacheEntry createUpdated( if (HttpCacheEntry.isNewer(entry, response)) { return entry; } + final String s = CacheKeyGenerator.getRequestUri(host, request); + final URI uri = CacheKeyGenerator.normalize(s); + final HeaderGroup requestHeaders = filterHopByHopHeaders(request); final HeaderGroup mergedHeaders = mergeHeaders(entry, response); return new HttpCacheEntry( requestInstant, responseInstant, - entry.getRequestMethod(), - entry.getRequestURI(), - headers(entry.requestHeaderIterator()), + request.getMethod(), + uri.toASCIIString(), + requestHeaders, entry.getStatus(), mergedHeaders, entry.getResource(), diff --git a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/BasicHttpAsyncCache.java b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/BasicHttpAsyncCache.java index 2e2568283e..e0da6e04fe 100644 --- a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/BasicHttpAsyncCache.java +++ b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/BasicHttpAsyncCache.java @@ -435,6 +435,8 @@ public Cancellable update( final HttpCacheEntry updatedEntry = cacheEntryFactory.createUpdated( requestSent, responseReceived, + host, + request, originResponse, stale.entry); final String variantKey = cacheKeyGenerator.generateVariantKey(request, updatedEntry); @@ -456,6 +458,8 @@ public Cancellable storeFromNegotiated( final HttpCacheEntry updatedEntry = cacheEntryFactory.createUpdated( requestSent, responseReceived, + host, + request, originResponse, negotiated.entry); diff --git a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/BasicHttpCache.java b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/BasicHttpCache.java index fe61c76117..b635610684 100644 --- a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/BasicHttpCache.java +++ b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/BasicHttpCache.java @@ -266,6 +266,8 @@ public CacheHit update( final HttpCacheEntry updatedEntry = cacheEntryFactory.createUpdated( requestSent, responseReceived, + host, + request, originResponse, stale.entry); final String variantKey = cacheKeyGenerator.generateVariantKey(request, updatedEntry); @@ -286,8 +288,10 @@ public CacheHit storeFromNegotiated( final HttpCacheEntry updatedEntry = cacheEntryFactory.createUpdated( requestSent, responseReceived, + host, + request, originResponse, - negotiated.entry); + negotiated.entry); storeInternal(negotiated.getEntryKey(), updatedEntry); final String rootKey = cacheKeyGenerator.generateKey(host, request); diff --git a/httpclient5-cache/src/test/java/org/apache/hc/client5/http/cache/TestHttpCacheEntryFactory.java b/httpclient5-cache/src/test/java/org/apache/hc/client5/http/cache/TestHttpCacheEntryFactory.java index 24adff6eae..96e3fdb0b8 100644 --- a/httpclient5-cache/src/test/java/org/apache/hc/client5/http/cache/TestHttpCacheEntryFactory.java +++ b/httpclient5-cache/src/test/java/org/apache/hc/client5/http/cache/TestHttpCacheEntryFactory.java @@ -211,7 +211,7 @@ public void entryIsStillUpdatedByResponseWithMalformedDate() throws Exception { @Test public void testUpdateCacheEntryReturnsDifferentEntryInstance() { entry = HttpTestUtils.makeCacheEntry(); - final HttpCacheEntry newEntry = impl.createUpdated(requestDate, responseDate, response, entry); + final HttpCacheEntry newEntry = impl.createUpdated(requestDate, responseDate, host, request, response, entry); Assertions.assertNotSame(newEntry, entry); } @@ -339,17 +339,23 @@ public void testCreateUpdatedResourceEntry() { new BasicHeader("Cache-Control", "public") ); - final HttpCacheEntry updatedEntry = impl.createUpdated(tenSecondsAgo, oneSecondAgo, response, entry); + request.setHeaders( + new BasicHeader("X-custom", "my other stuff"), + new BasicHeader(HttpHeaders.ACCEPT, "stuff, other-stuff"), + new BasicHeader(HttpHeaders.ACCEPT_LANGUAGE, "en, de") + ); + + final HttpCacheEntry updatedEntry = impl.createUpdated(tenSecondsAgo, oneSecondAgo, host, request, response, entry); MatcherAssert.assertThat(updatedEntry, HttpCacheEntryMatcher.equivalent( HttpTestUtils.makeCacheEntry( tenSecondsAgo, oneSecondAgo, Method.GET, - "/stuff", + "http://foo.example.com:80/stuff", new Header[]{ - new BasicHeader("X-custom", "my stuff"), - new BasicHeader(HttpHeaders.ACCEPT, "stuff"), + new BasicHeader("X-custom", "my other stuff"), + new BasicHeader(HttpHeaders.ACCEPT, "stuff, other-stuff"), new BasicHeader(HttpHeaders.ACCEPT_LANGUAGE, "en, de") }, HttpStatus.SC_NOT_MODIFIED, @@ -374,7 +380,7 @@ public void testUpdateNotModifiedIfResponseOlder() { response.setHeader("Date", DateUtils.formatStandardDate(tenSecondsAgo)); response.setHeader("ETag", "\"old-etag\""); - final HttpCacheEntry newEntry = impl.createUpdated(Instant.now(), Instant.now(), response, entry); + final HttpCacheEntry newEntry = impl.createUpdated(Instant.now(), Instant.now(), host, request, response, entry); Assertions.assertSame(newEntry, entry); } @@ -382,7 +388,7 @@ public void testUpdateNotModifiedIfResponseOlder() { @Test public void testUpdateHasLatestRequestAndResponseDates() { entry = HttpTestUtils.makeCacheEntry(tenSecondsAgo, eightSecondsAgo); - final HttpCacheEntry updated = impl.createUpdated(twoSecondsAgo, oneSecondAgo, response, entry); + final HttpCacheEntry updated = impl.createUpdated(twoSecondsAgo, oneSecondAgo, host, request, response, entry); Assertions.assertEquals(twoSecondsAgo, updated.getRequestInstant()); Assertions.assertEquals(oneSecondAgo, updated.getResponseInstant()); @@ -393,7 +399,7 @@ public void cannotUpdateFromANon304OriginResponse() throws Exception { entry = HttpTestUtils.makeCacheEntry(); response = new BasicHttpResponse(HttpStatus.SC_OK, "OK"); Assertions.assertThrows(IllegalArgumentException.class, () -> - impl.createUpdated(Instant.now(), Instant.now(), response, entry)); + impl.createUpdated(Instant.now(), Instant.now(), host, request, response, entry)); } } From c7ba2151f4a6061e891689f43ca9486170d8bfa9 Mon Sep 17 00:00:00 2001 From: Oleg Kalnichevski Date: Mon, 23 Oct 2023 10:14:39 +0200 Subject: [PATCH 2/8] HTTPCLIENT-2277: Do not store AUTHORIZATION request header in the cache entry per RFC 9111 section 3.5 --- .../apache/hc/client5/http/cache/HttpCacheEntryFactory.java | 4 ++++ .../hc/client5/http/cache/TestHttpCacheEntryFactory.java | 3 ++- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/cache/HttpCacheEntryFactory.java b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/cache/HttpCacheEntryFactory.java index 91c6ae26ad..46a8349e64 100644 --- a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/cache/HttpCacheEntryFactory.java +++ b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/cache/HttpCacheEntryFactory.java @@ -212,6 +212,8 @@ public HttpCacheEntry create(final Instant requestInstant, final String s = CacheKeyGenerator.getRequestUri(host, request); final URI uri = CacheKeyGenerator.normalize(s); final HeaderGroup requestHeaders = filterHopByHopHeaders(request); + // Strip AUTHORIZATION from request headers + requestHeaders.removeHeaders(HttpHeaders.AUTHORIZATION); final HeaderGroup responseHeaders = filterHopByHopHeaders(response); ensureDate(responseHeaders, responseInstant); return new HttpCacheEntry( @@ -256,6 +258,8 @@ public HttpCacheEntry createUpdated( final String s = CacheKeyGenerator.getRequestUri(host, request); final URI uri = CacheKeyGenerator.normalize(s); final HeaderGroup requestHeaders = filterHopByHopHeaders(request); + // Strip AUTHORIZATION from request headers + requestHeaders.removeHeaders(HttpHeaders.AUTHORIZATION); final HeaderGroup mergedHeaders = mergeHeaders(entry, response); return new HttpCacheEntry( requestInstant, diff --git a/httpclient5-cache/src/test/java/org/apache/hc/client5/http/cache/TestHttpCacheEntryFactory.java b/httpclient5-cache/src/test/java/org/apache/hc/client5/http/cache/TestHttpCacheEntryFactory.java index 96e3fdb0b8..7b2bd7ed5e 100644 --- a/httpclient5-cache/src/test/java/org/apache/hc/client5/http/cache/TestHttpCacheEntryFactory.java +++ b/httpclient5-cache/src/test/java/org/apache/hc/client5/http/cache/TestHttpCacheEntryFactory.java @@ -273,7 +273,8 @@ public void testCreateResourceEntry() { new BasicHeader("Keep-Alive", "timeout, max=20"), new BasicHeader("X-custom", "my stuff"), new BasicHeader(HttpHeaders.ACCEPT, "stuff"), - new BasicHeader(HttpHeaders.ACCEPT_LANGUAGE, "en, de") + new BasicHeader(HttpHeaders.ACCEPT_LANGUAGE, "en, de"), + new BasicHeader(HttpHeaders.AUTHORIZATION, "Super secret") ); response.setHeaders( new BasicHeader(HttpHeaders.TRANSFER_ENCODING, "identity"), From c19cfe3a46231bf11e6c3bdc6f6843aef8eaea64 Mon Sep 17 00:00:00 2001 From: Oleg Kalnichevski Date: Sun, 22 Oct 2023 12:07:31 +0200 Subject: [PATCH 3/8] HTTPCLIENT-2277: Aligned ResponseCachingPolicy with the specification requirements per RFC 9111 section 3 --- .../http/impl/cache/AsyncCachingExec.java | 5 + .../client5/http/impl/cache/CachingExec.java | 4 + .../http/impl/cache/CachingExecBase.java | 14 +- .../impl/cache/ResponseCachingPolicy.java | 220 ++++++------ .../impl/cache/TestResponseCachingPolicy.java | 333 ++++++------------ 5 files changed, 232 insertions(+), 344 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 5608bbfba7..f30eeff1f4 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 @@ -462,6 +462,11 @@ public void cancelled() { } }); + if (isResponseTooBig(entityDetails)) { + LOG.debug("Backend response is known to be too big"); + return asyncExecCallback.handleResponse(backendResponse, entityDetails); + } + final ResponseCacheControl responseCacheControl = CacheControlHeaderParser.INSTANCE.parse(backendResponse); final boolean cacheable = responseCachingPolicy.isResponseCacheable(responseCacheControl, request, backendResponse); if (cacheable) { 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 30467ed202..6311d8865f 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 @@ -368,6 +368,10 @@ ClassicHttpResponse handleBackendResponse( final ClassicHttpResponse backendResponse) throws IOException { responseCache.evictInvalidatedEntries(target, request, backendResponse); + if (isResponseTooBig(backendResponse.getEntity())) { + LOG.debug("Backend response is known to be too big"); + return backendResponse; + } final ResponseCacheControl responseCacheControl = CacheControlHeaderParser.INSTANCE.parse(backendResponse); final boolean cacheable = responseCachingPolicy.isResponseCacheable(responseCacheControl, request, backendResponse); if (cacheable) { diff --git a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachingExecBase.java b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachingExecBase.java index 4c7f09b718..81bb1e4c31 100644 --- a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachingExecBase.java +++ b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachingExecBase.java @@ -35,6 +35,7 @@ import org.apache.hc.client5.http.cache.HttpCacheContext; import org.apache.hc.client5.http.cache.HttpCacheEntry; import org.apache.hc.client5.http.cache.ResourceIOException; +import org.apache.hc.core5.http.EntityDetails; import org.apache.hc.core5.http.Header; import org.apache.hc.core5.http.HttpHeaders; import org.apache.hc.core5.http.HttpHost; @@ -85,7 +86,6 @@ public class CachingExecBase { this.cacheableRequestPolicy = new CacheableRequestPolicy(); this.suitabilityChecker = new CachedResponseSuitabilityChecker(this.validityPolicy, this.cacheConfig); this.responseCachingPolicy = new ResponseCachingPolicy( - this.cacheConfig.getMaxObjectSize(), this.cacheConfig.isSharedCache(), this.cacheConfig.isNeverCacheHTTP10ResponsesWithQuery(), this.cacheConfig.isNeverCacheHTTP11ResponsesWithQuery(), @@ -282,4 +282,16 @@ void storeRequestIfModifiedSinceFor304Response(final HttpRequest request, final } } + boolean isResponseTooBig(final EntityDetails entityDetails) { + if (entityDetails == null) { + return false; + } + final long contentLength = entityDetails.getContentLength(); + if (contentLength == -1) { + return false; + } + final long maxObjectSize = cacheConfig.getMaxObjectSize(); + return contentLength > maxObjectSize; + } + } diff --git a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/ResponseCachingPolicy.java b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/ResponseCachingPolicy.java index cf3e83611b..47472f06d6 100644 --- a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/ResponseCachingPolicy.java +++ b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/ResponseCachingPolicy.java @@ -33,8 +33,6 @@ import org.apache.hc.client5.http.cache.HttpCacheEntry; import org.apache.hc.client5.http.utils.DateUtils; -import org.apache.hc.core5.http.Header; -import org.apache.hc.core5.http.HeaderElement; import org.apache.hc.core5.http.HttpHeaders; import org.apache.hc.core5.http.HttpRequest; import org.apache.hc.core5.http.HttpResponse; @@ -43,7 +41,6 @@ import org.apache.hc.core5.http.Method; import org.apache.hc.core5.http.ProtocolVersion; import org.apache.hc.core5.http.message.BasicTokenIterator; -import org.apache.hc.core5.http.message.MessageSupport; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -63,7 +60,6 @@ class ResponseCachingPolicy { private static final Logger LOG = LoggerFactory.getLogger(ResponseCachingPolicy.class); - private final long maxObjectSizeBytes; private final boolean sharedCache; private final boolean neverCache1_0ResponsesWithQueryString; private final boolean neverCache1_1ResponsesWithQueryString; @@ -79,8 +75,6 @@ class ResponseCachingPolicy { /** * Constructs a new ResponseCachingPolicy with the specified cache policy settings and stale-if-error support. * - * @param maxObjectSizeBytes the maximum size of objects, in bytes, that should be stored - * in the cache * @param sharedCache whether to behave as a shared cache (true) or a * non-shared/private cache (false) * @param neverCache1_0ResponsesWithQueryString {@code true} to never cache HTTP 1.0 responses with a query string, @@ -92,110 +86,117 @@ class ResponseCachingPolicy { * results in an error, {@code false} to disable this feature. * @since 5.3 */ - public ResponseCachingPolicy(final long maxObjectSizeBytes, + public ResponseCachingPolicy( final boolean sharedCache, final boolean neverCache1_0ResponsesWithQueryString, final boolean neverCache1_1ResponsesWithQueryString, final boolean staleIfErrorEnabled) { - this.maxObjectSizeBytes = maxObjectSizeBytes; this.sharedCache = sharedCache; this.neverCache1_0ResponsesWithQueryString = neverCache1_0ResponsesWithQueryString; this.neverCache1_1ResponsesWithQueryString = neverCache1_1ResponsesWithQueryString; this.staleIfErrorEnabled = staleIfErrorEnabled; } - boolean isResponseCacheable(final ResponseCacheControl cacheControl, final String httpMethod, final HttpResponse response) { - if (response.countHeaders(HttpHeaders.EXPIRES) > 1) { - LOG.debug("Multiple Expires headers"); + /** + * Determine if the {@link HttpResponse} gotten from the origin is a + * cacheable response. + * + * @return {@code true} if response is cacheable + */ + public boolean isResponseCacheable(final ResponseCacheControl cacheControl, final HttpRequest request, final HttpResponse response) { + final ProtocolVersion version = request.getVersion() != null ? request.getVersion() : HttpVersion.DEFAULT; + if (version.compareToVersion(HttpVersion.HTTP_1_1) > 0) { + if (LOG.isDebugEnabled()) { + LOG.debug("Protocol version {} is non-cacheable", version); + } return false; } - if (response.countHeaders(HttpHeaders.DATE) > 1) { - LOG.debug("Multiple Date headers"); + // Presently only GET and HEAD methods are supported + final String httpMethod = request.getMethod(); + if (!Method.GET.isSame(httpMethod) && !Method.HEAD.isSame(httpMethod)) { + if (LOG.isDebugEnabled()) { + LOG.debug("{} method response is not cacheable", httpMethod); + } return false; } - final Instant responseDate = DateUtils.parseStandardDate(response, HttpHeaders.DATE); - final Instant responseExpires = DateUtils.parseStandardDate(response, HttpHeaders.EXPIRES); + final int code = response.getCode(); - if (expiresHeaderLessOrEqualToDateHeaderAndNoCacheControl(cacheControl, responseDate, responseExpires)) { - LOG.debug("Expires header less or equal to Date header and no cache control directives"); + // Should never happen but better be defensive + if (code <= HttpStatus.SC_INFORMATIONAL) { return false; } - boolean cacheable = false; - - if (!Method.GET.isSame(httpMethod) && !Method.HEAD.isSame(httpMethod) && !Method.POST.isSame((httpMethod))) { + if (isKnownNonCacheableStatusCode(code)) { if (LOG.isDebugEnabled()) { - LOG.debug("{} method response is not cacheable", httpMethod); + LOG.debug("{} response is not cacheable", code); } return false; } - final int status = response.getCode(); - if (isKnownCacheableStatusCode(status)) { - // these response codes MAY be cached - cacheable = true; - } else if (isKnownNonCacheableStatusCode(status)) { - if (LOG.isDebugEnabled()) { - LOG.debug("{} response is not cacheable", status); + if (request.getPath().contains("?")) { + if (neverCache1_0ResponsesWithQueryString && from1_0Origin(response)) { + LOG.debug("Response is not cacheable as it had a query string"); + return false; + } else if (!neverCache1_1ResponsesWithQueryString && !isExplicitlyCacheable(cacheControl, response)) { + LOG.debug("Response is not cacheable as it is missing explicit caching headers"); + return false; } + } + + if (cacheControl.isMustUnderstand() && !understoodStatusCode(code)) { + // must-understand cache directive overrides no-store + LOG.debug("Response contains a status code that the cache does not understand, so it's not cacheable"); return false; - } else if (isUnknownStatusCode(status)) { - // a response with an unknown status code MUST NOT be - // cached - if (LOG.isDebugEnabled()) { - LOG.debug("{} response is unknown", status); - } + } + + if (isExplicitlyNonCacheable(cacheControl)) { + LOG.debug("Response is explicitly non-cacheable per cache control directive"); return false; } - final Header contentLength = response.getFirstHeader(HttpHeaders.CONTENT_LENGTH); - if (contentLength != null) { - final long contentLengthValue = Long.parseLong(contentLength.getValue()); - if (contentLengthValue > this.maxObjectSizeBytes) { - if (LOG.isDebugEnabled()) { - LOG.debug("Response content length exceeds {}", this.maxObjectSizeBytes); - } + if (sharedCache) { + if (request.containsHeader(HttpHeaders.AUTHORIZATION) && + cacheControl.getSharedMaxAge() == -1 && + !cacheControl.isPublic()) { + LOG.debug("Request contains private credentials"); return false; } } - final Iterator it = MessageSupport.iterate(response, HttpHeaders.VARY); - while (it.hasNext()) { - final HeaderElement elem = it.next(); - if ("*".equals(elem.getName())) { - if (LOG.isDebugEnabled()) { - LOG.debug("Vary * found"); - } - return false; - } + // See if the response is tainted + if (response.countHeaders(HttpHeaders.EXPIRES) > 1) { + LOG.debug("Multiple Expires headers"); + return false; } - if (isExplicitlyNonCacheable(cacheControl)) { - LOG.debug("Response is explicitly non-cacheable"); + + if (response.countHeaders(HttpHeaders.DATE) > 1) { + LOG.debug("Multiple Date headers"); return false; } - final Duration freshnessLifetime = calculateFreshnessLifetime(cacheControl, responseDate, responseExpires); + final Instant responseDate = DateUtils.parseStandardDate(response, HttpHeaders.DATE); + final Instant responseExpires = DateUtils.parseStandardDate(response, HttpHeaders.EXPIRES); - // If the 'immutable' directive is present and the response is still fresh, - // then the response is considered cacheable without further validation - if (cacheControl.isImmutable() && responseIsStillFresh(responseDate, freshnessLifetime)) { - if (LOG.isDebugEnabled()) { - LOG.debug("Response is immutable and fresh, considered cacheable without further validation"); - } - return true; + if (expiresHeaderLessOrEqualToDateHeaderAndNoCacheControl(cacheControl, responseDate, responseExpires)) { + LOG.debug("Expires header less or equal to Date header and no cache control directives"); + return false; } - // calculate freshness lifetime - if (freshnessLifetime.isNegative() || freshnessLifetime.isZero()) { - if (LOG.isDebugEnabled()) { - LOG.debug("Freshness lifetime is invalid"); + // Treat responses with `Vary: *` as essentially non-cacheable. + final Iterator it = new BasicTokenIterator(response.headerIterator(HttpHeaders.VARY)); + while (it.hasNext()) { + final String token = it.next(); + if ("*".equals(token)) { + if (LOG.isDebugEnabled()) { + LOG.debug("Vary: * found"); + } + return false; } - return false; } - return cacheable || isExplicitlyCacheable(cacheControl, response); + return isExplicitlyCacheable(cacheControl, response) || isHeuristicallyCacheable(cacheControl, code, responseDate, responseExpires); } private static boolean isKnownCacheableStatusCode(final int status) { @@ -253,60 +254,57 @@ protected boolean isExplicitlyNonCacheable(final ResponseCacheControl cacheContr } protected boolean isExplicitlyCacheable(final ResponseCacheControl cacheControl, final HttpResponse response) { - if (response.containsHeader(HttpHeaders.EXPIRES)) { + if (cacheControl.isPublic()) { return true; } - return cacheControl.getMaxAge() > 0 || cacheControl.getSharedMaxAge()>0 || - cacheControl.isMustRevalidate() || cacheControl.isProxyRevalidate() || (cacheControl.isPublic()); - } - - /** - * Determine if the {@link HttpResponse} gotten from the origin is a - * cacheable response. - * - * @return {@code true} if response is cacheable - */ - public boolean isResponseCacheable(final ResponseCacheControl cacheControl, final HttpRequest request, final HttpResponse response) { - final ProtocolVersion version = request.getVersion() != null ? request.getVersion() : HttpVersion.DEFAULT; - if (version.compareToVersion(HttpVersion.HTTP_1_1) > 0) { - if (LOG.isDebugEnabled()) { - LOG.debug("Protocol version {} is non-cacheable", version); - } - return false; + if (!sharedCache && cacheControl.isCachePrivate()) { + return true; } - - if (cacheControl.isMustUnderstand() && cacheControl.isNoStore() && !understoodStatusCode(response.getCode())) { - // must-understand cache directive overrides no-store - LOG.debug("Response contains a status code that the cache does not understand, so it's not cacheable"); - return false; + if (response.containsHeader(HttpHeaders.EXPIRES)) { + return true; } - - if (!cacheControl.isMustUnderstand() && cacheControl.isNoStore()) { - LOG.debug("Response is explicitly non-cacheable per cache control directive"); - return false; + if (cacheControl.getMaxAge() > 0) { + return true; } - - - if (request.getRequestUri().contains("?")) { - if (neverCache1_0ResponsesWithQueryString && from1_0Origin(response)) { - LOG.debug("Response is not cacheable as it had a query string"); - return false; - } else if (!neverCache1_1ResponsesWithQueryString && !isExplicitlyCacheable(cacheControl, response)) { - LOG.debug("Response is not cacheable as it is missing explicit caching headers"); - return false; - } + if (sharedCache && cacheControl.getSharedMaxAge() > 0) { + return true; } + return false; + } - if (sharedCache) { - if (request.countHeaders(HttpHeaders.AUTHORIZATION) > 0 - && !(cacheControl.getSharedMaxAge() > -1 || cacheControl.isMustRevalidate() || cacheControl.isPublic())) { - LOG.debug("Request contains private credentials"); + protected boolean isHeuristicallyCacheable(final ResponseCacheControl cacheControl, + final int status, + final Instant responseDate, + final Instant responseExpires) { + if (isKnownCacheableStatusCode(status)) { + final Duration freshnessLifetime = calculateFreshnessLifetime(cacheControl, responseDate, responseExpires); + // calculate freshness lifetime + if (freshnessLifetime.isNegative()) { + if (LOG.isDebugEnabled()) { + LOG.debug("Freshness lifetime is invalid"); + } return false; } + // If the 'immutable' directive is present and the response is still fresh, + // then the response is considered cacheable without further validation + if (cacheControl.isImmutable() && responseIsStillFresh(responseDate, freshnessLifetime)) { + if (LOG.isDebugEnabled()) { + LOG.debug("Response is immutable and fresh, considered cacheable without further validation"); + } + return true; + } + if (freshnessLifetime.compareTo(Duration.ZERO) > 0) { + return true; + } + } else if (isUnknownStatusCode(status)) { + // a response with an unknown status code MUST NOT be + // cached + if (LOG.isDebugEnabled()) { + LOG.debug("{} response is unknown", status); + } + return false; } - - final String method = request.getMethod(); - return isResponseCacheable(cacheControl, method, response); + return false; } private boolean expiresHeaderLessOrEqualToDateHeaderAndNoCacheControl(final ResponseCacheControl cacheControl, final Instant responseDate, final Instant expires) { @@ -316,7 +314,7 @@ private boolean expiresHeaderLessOrEqualToDateHeaderAndNoCacheControl(final Resp if (expires == null || responseDate == null) { return false; } - return expires.equals(responseDate) || expires.isBefore(responseDate); + return expires.compareTo(responseDate) <= 0; } private boolean from1_0Origin(final HttpResponse response) { diff --git a/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestResponseCachingPolicy.java b/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestResponseCachingPolicy.java index 570900a7ba..9215e5c184 100644 --- a/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestResponseCachingPolicy.java +++ b/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestResponseCachingPolicy.java @@ -42,6 +42,7 @@ import org.apache.hc.core5.http.HttpResponse; import org.apache.hc.core5.http.HttpStatus; import org.apache.hc.core5.http.HttpVersion; +import org.apache.hc.core5.http.Method; import org.apache.hc.core5.http.message.BasicHttpRequest; import org.apache.hc.core5.http.message.BasicHttpResponse; import org.junit.jupiter.api.Assertions; @@ -67,7 +68,7 @@ public void setUp() throws Exception { sixSecondsAgo = now.minusSeconds(6); tenSecondsFromNow = now.plusSeconds(10); - policy = new ResponseCachingPolicy(0, true, false, false, false); + policy = new ResponseCachingPolicy(true, false, false, false); request = new BasicHttpRequest("GET","/"); response = new BasicHttpResponse(HttpStatus.SC_OK, ""); response.setHeader("Date", DateUtils.formatStandardDate(Instant.now())); @@ -76,14 +77,26 @@ public void setUp() throws Exception { } @Test - public void testIsGetCacheable() { - Assertions.assertTrue(policy.isResponseCacheable(responseCacheControl, "GET", response)); + public void testGetCacheable() { + policy = new ResponseCachingPolicy(true, false, false, true); + request = new BasicHttpRequest(Method.GET, "/"); + Assertions.assertTrue(policy.isResponseCacheable(responseCacheControl, request, response)); + } + + @Test + public void testHeadCacheable() { + policy = new ResponseCachingPolicy(true, false, false, true); + request = new BasicHttpRequest(Method.HEAD, "/"); + Assertions.assertTrue(policy.isResponseCacheable(responseCacheControl, request, response)); } @Test - public void testIsHeadCacheable() { - policy = new ResponseCachingPolicy(0, true, false, false, true); - Assertions.assertTrue(policy.isResponseCacheable(responseCacheControl, "HEAD", response)); + public void testArbitraryMethodNotCacheable() { + request = new BasicHttpRequest("PUT", "/"); + Assertions.assertFalse(policy.isResponseCacheable(responseCacheControl, request, response)); + + request = new BasicHttpRequest("huh", "/"); + Assertions.assertFalse(policy.isResponseCacheable(responseCacheControl, request, response)); } @Test @@ -95,7 +108,7 @@ public void testResponsesToRequestsWithAuthorizationHeadersAreNotCacheableByShar @Test public void testResponsesToRequestsWithAuthorizationHeadersAreCacheableByNonSharedCache() { - policy = new ResponseCachingPolicy(0, false, false, false, true); + policy = new ResponseCachingPolicy(false, false, false, true); request = new BasicHttpRequest("GET","/"); request.setHeader("Authorization", StandardAuthScheme.BASIC + " dXNlcjpwYXNzd2Q="); Assertions.assertTrue(policy.isResponseCacheable(responseCacheControl, request, response)); @@ -112,16 +125,6 @@ public void testAuthorizedResponsesWithSMaxAgeAreCacheable() { Assertions.assertTrue(policy.isResponseCacheable(responseCacheControl, request, response)); } - @Test - public void testAuthorizedResponsesWithMustRevalidateAreCacheable() { - request = new BasicHttpRequest("GET","/"); - request.setHeader("Authorization", StandardAuthScheme.BASIC + " dXNlcjpwYXNzd2Q="); - responseCacheControl = ResponseCacheControl.builder() - .setMustRevalidate(true) - .build(); - Assertions.assertTrue(policy.isResponseCacheable(responseCacheControl, request, response)); - } - @Test public void testAuthorizedResponsesWithCacheControlPublicAreCacheable() { request = new BasicHttpRequest("GET","/"); @@ -145,65 +148,53 @@ public void testAuthorizedResponsesWithCacheControlMaxAgeAreNotCacheable() { @Test public void test203ResponseCodeIsCacheable() { response.setCode(HttpStatus.SC_NON_AUTHORITATIVE_INFORMATION); - Assertions.assertTrue(policy.isResponseCacheable(responseCacheControl, "GET", response)); + Assertions.assertTrue(policy.isResponseCacheable(responseCacheControl, request, response)); } @Test public void test206ResponseCodeIsNotCacheable() { response.setCode(HttpStatus.SC_PARTIAL_CONTENT); - Assertions.assertFalse(policy.isResponseCacheable(responseCacheControl, "GET", response)); - } - - @Test - public void test206ResponseCodeIsNotCacheableUsingSharedPublicCache() { - policy = new ResponseCachingPolicy(0, true, false, false, true); - - request.setHeader("Authorization", StandardAuthScheme.BASIC + " QWxhZGRpbjpvcGVuIHNlc2FtZQ=="); - response.setCode(HttpStatus.SC_PARTIAL_CONTENT); - responseCacheControl = ResponseCacheControl.builder() - .setCachePublic(true) - .build(); Assertions.assertFalse(policy.isResponseCacheable(responseCacheControl, request, response)); } @Test public void test300ResponseCodeIsCacheable() { response.setCode(HttpStatus.SC_MULTIPLE_CHOICES); - Assertions.assertTrue(policy.isResponseCacheable(responseCacheControl, "GET", response)); + Assertions.assertTrue(policy.isResponseCacheable(responseCacheControl, request, response)); } @Test public void test301ResponseCodeIsCacheable() { response.setCode(HttpStatus.SC_MOVED_PERMANENTLY); - Assertions.assertTrue(policy.isResponseCacheable(responseCacheControl, "GET", response)); + Assertions.assertTrue(policy.isResponseCacheable(responseCacheControl, request, response)); } @Test public void test410ResponseCodeIsCacheable() { response.setCode(HttpStatus.SC_GONE); - Assertions.assertTrue(policy.isResponseCacheable(responseCacheControl, "GET", response)); + Assertions.assertTrue(policy.isResponseCacheable(responseCacheControl, request, response)); } @Test public void testPlain302ResponseCodeIsNotCacheable() { response.setCode(HttpStatus.SC_MOVED_TEMPORARILY); response.removeHeaders("Expires"); - Assertions.assertFalse(policy.isResponseCacheable(responseCacheControl, "GET", response)); + Assertions.assertFalse(policy.isResponseCacheable(responseCacheControl, request, response)); } @Test public void testPlain303ResponseCodeIsNotCacheableUnderDefaultBehavior() { response.setCode(HttpStatus.SC_SEE_OTHER); response.removeHeaders("Expires"); - Assertions.assertFalse(policy.isResponseCacheable(responseCacheControl, "GET", response)); + Assertions.assertFalse(policy.isResponseCacheable(responseCacheControl, request, response)); } @Test public void testPlain303ResponseCodeIsNotCacheableEvenIf303CachingEnabled() { - policy = new ResponseCachingPolicy(0, true, false, true, true); + policy = new ResponseCachingPolicy(true, false, true, true); response.setCode(HttpStatus.SC_SEE_OTHER); response.removeHeaders("Expires"); - Assertions.assertFalse(policy.isResponseCacheable(responseCacheControl, "GET", response)); + Assertions.assertFalse(policy.isResponseCacheable(responseCacheControl, request, response)); } @@ -211,7 +202,7 @@ public void testPlain303ResponseCodeIsNotCacheableEvenIf303CachingEnabled() { public void testPlain307ResponseCodeIsNotCacheable() { response.setCode(HttpStatus.SC_TEMPORARY_REDIRECT); response.removeHeaders("Expires"); - Assertions.assertFalse(policy.isResponseCacheable(responseCacheControl, "GET", response)); + Assertions.assertFalse(policy.isResponseCacheable(responseCacheControl, request, response)); } @Test @@ -219,7 +210,7 @@ public void testNon206WithExplicitExpiresIsCacheable() { final int status = getRandomStatus(); response.setCode(status); response.setHeader("Expires", DateUtils.formatStandardDate(Instant.now().plus(1, ChronoUnit.HOURS))); - Assertions.assertTrue(policy.isResponseCacheable(responseCacheControl, "GET", response)); + Assertions.assertTrue(policy.isResponseCacheable(responseCacheControl, request, response)); } @Test @@ -229,7 +220,7 @@ public void testNon206WithMaxAgeIsCacheable() { responseCacheControl = ResponseCacheControl.builder() .setMaxAge(0) .build(); - Assertions.assertFalse(policy.isResponseCacheable(responseCacheControl, "GET", response)); + Assertions.assertFalse(policy.isResponseCacheable(responseCacheControl, request, response)); } @Test @@ -237,7 +228,7 @@ public void testMissingCacheControlHeader() { final int status = getRandomStatus(); response.setCode(status); response.removeHeaders(HttpHeaders.CACHE_CONTROL); - Assertions.assertTrue(policy.isResponseCacheable(responseCacheControl, "GET", response)); + Assertions.assertTrue(policy.isResponseCacheable(responseCacheControl, request, response)); } @Test @@ -247,7 +238,7 @@ public void testNon206WithSMaxAgeIsCacheable() { responseCacheControl = ResponseCacheControl.builder() .setSharedMaxAge(1) .build(); - Assertions.assertTrue(policy.isResponseCacheable(responseCacheControl, "GET", response)); + Assertions.assertTrue(policy.isResponseCacheable(responseCacheControl, request, response)); } @Test @@ -257,7 +248,7 @@ public void testNon206WithMustRevalidateIsCacheable() { responseCacheControl = ResponseCacheControl.builder() .setMustRevalidate(true) .build(); - Assertions.assertTrue(policy.isResponseCacheable(responseCacheControl, "GET", response)); + Assertions.assertTrue(policy.isResponseCacheable(responseCacheControl, request, response)); } @Test @@ -267,7 +258,7 @@ public void testNon206WithProxyRevalidateIsCacheable() { responseCacheControl = ResponseCacheControl.builder() .setProxyRevalidate(true) .build(); - Assertions.assertTrue(policy.isResponseCacheable(responseCacheControl, "GET", response)); + Assertions.assertTrue(policy.isResponseCacheable(responseCacheControl, request, response)); } @Test @@ -277,7 +268,7 @@ public void testNon206WithPublicCacheControlIsCacheable() { responseCacheControl = ResponseCacheControl.builder() .setCachePublic(true) .build(); - Assertions.assertTrue(policy.isResponseCacheable(responseCacheControl, "GET", response)); + Assertions.assertTrue(policy.isResponseCacheable(responseCacheControl, request, response)); } @Test @@ -287,164 +278,86 @@ public void testNon206WithPrivateCacheControlIsNotCacheableBySharedCache() { responseCacheControl = ResponseCacheControl.builder() .setCachePrivate(true) .build(); - Assertions.assertFalse(policy.isResponseCacheable(responseCacheControl, "GET", response)); + Assertions.assertFalse(policy.isResponseCacheable(responseCacheControl, request, response)); } @Test public void test200ResponseWithPrivateCacheControlIsCacheableByNonSharedCache() { - policy = new ResponseCachingPolicy(0, false, false, false, true); + policy = new ResponseCachingPolicy(false, false, false, true); response.setCode(HttpStatus.SC_OK); responseCacheControl = ResponseCacheControl.builder() .setCachePrivate(true) .build(); - Assertions.assertTrue(policy.isResponseCacheable(responseCacheControl, "GET", response)); - } - - @Test - public void testIsGetWithNoCacheCacheable() { - responseCacheControl = ResponseCacheControl.builder() - .setNoCache(true) - .build(); - - Assertions.assertTrue(policy.isResponseCacheable(responseCacheControl, "GET", response)); + Assertions.assertTrue(policy.isResponseCacheable(responseCacheControl, request, response)); } @Test - public void testIsHeadWithNoCacheCacheable() { + public void testControlNoCacheCacheable() { responseCacheControl = ResponseCacheControl.builder() .setNoCache(true) .build(); - Assertions.assertTrue(policy.isResponseCacheable(responseCacheControl, "HEAD", response)); - } - - @Test - public void testIsGetWithNoStoreCacheable() { - responseCacheControl = ResponseCacheControl.builder() - .setNoStore(true) - .build(); - - Assertions.assertFalse(policy.isResponseCacheable(responseCacheControl, "GET", response)); - } - - @Test - public void testIsHeadWithNoStoreCacheable() { - responseCacheControl = ResponseCacheControl.builder() - .setNoStore(true) - .build(); - - Assertions.assertFalse(policy.isResponseCacheable(responseCacheControl, "HEAD", response)); + Assertions.assertTrue(policy.isResponseCacheable(responseCacheControl, request, response)); } @Test - public void testIsGetWithNoStoreEmbeddedInListCacheable() { + public void testControlNoStoreNotCacheable() { responseCacheControl = ResponseCacheControl.builder() - .setCachePublic(true) .setNoStore(true) .build(); - Assertions.assertFalse(policy.isResponseCacheable(responseCacheControl, "GET", response)); + Assertions.assertFalse(policy.isResponseCacheable(responseCacheControl, request, response)); } @Test - public void testIsHeadWithNoStoreEmbeddedInListCacheable() { + public void testControlNoStoreEmbeddedInListCacheable() { responseCacheControl = ResponseCacheControl.builder() .setCachePublic(true) .setNoStore(true) .build(); - Assertions.assertFalse(policy.isResponseCacheable(responseCacheControl, "HEAD", response)); - } - - @Test - public void testIsGetWithNoCacheEmbeddedInListCacheable() { - responseCacheControl = ResponseCacheControl.builder() - .setCachePublic(true) - .setNoCache(true) - .build(); - - Assertions.assertTrue(policy.isResponseCacheable(responseCacheControl, "GET", response)); - } - - @Test - public void testIsHeadWithNoCacheEmbeddedInListCacheable() { - responseCacheControl = ResponseCacheControl.builder() - .setCachePublic(true) - .setNoCache(true) - .build(); - - Assertions.assertTrue(policy.isResponseCacheable(responseCacheControl, "HEAD", response)); + Assertions.assertFalse(policy.isResponseCacheable(responseCacheControl, request, response)); } @Test - public void testIsGetWithNoCacheEmbeddedInListAfterFirstHeaderCacheable() { + public void testControlNoCacheEmbeddedInListCacheable() { responseCacheControl = ResponseCacheControl.builder() - .setMaxAge(20) .setCachePublic(true) .setNoCache(true) .build(); - Assertions.assertTrue(policy.isResponseCacheable(responseCacheControl, "GET", response)); + Assertions.assertTrue(policy.isResponseCacheable(responseCacheControl, request, response)); } @Test - public void testIsHeadWithNoCacheEmbeddedInListAfterFirstHeaderCacheable() { + public void testControlNoCacheEmbeddedInListAfterFirstHeaderCacheable() { responseCacheControl = ResponseCacheControl.builder() .setMaxAge(20) .setCachePublic(true) .setNoCache(true) .build(); - Assertions.assertTrue(policy.isResponseCacheable(responseCacheControl, "HEAD", response)); - } - - @Test - public void testIsGetWithNoStoreEmbeddedInListAfterFirstHeaderCacheable() { - responseCacheControl = ResponseCacheControl.builder() - .setMaxAge(20) - .setCachePublic(true) - .setNoStore(true) - .build(); - - Assertions.assertFalse(policy.isResponseCacheable(responseCacheControl, "GET", response)); + Assertions.assertTrue(policy.isResponseCacheable(responseCacheControl, request, response)); } @Test - public void testIsHeadWithNoStoreEmbeddedInListAfterFirstHeaderCacheable() { + public void testControlNoStoreEmbeddedInListAfterFirstHeaderCacheable() { responseCacheControl = ResponseCacheControl.builder() .setMaxAge(20) .setCachePublic(true) .setNoStore(true) .build(); - Assertions.assertFalse(policy.isResponseCacheable(responseCacheControl, "HEAD", response)); - } - - @Test - public void testIsGetWithAnyCacheControlCacheable() { - responseCacheControl = ResponseCacheControl.builder() - .setMaxAge(10) - .build(); - - Assertions.assertTrue(policy.isResponseCacheable(responseCacheControl, "GET", response)); - - response = new BasicHttpResponse(HttpStatus.SC_OK, ""); - response.setHeader("Date", DateUtils.formatStandardDate(Instant.now())); - response.setHeader("Content-Length", "0"); - responseCacheControl = ResponseCacheControl.builder() - .build(); - - Assertions.assertTrue(policy.isResponseCacheable(responseCacheControl, "GET", response)); + Assertions.assertFalse(policy.isResponseCacheable(responseCacheControl, request, response)); } @Test - public void testIsHeadWithAnyCacheControlCacheable() { - policy = new ResponseCachingPolicy(0, true, false, false, true); + public void testControlAnyCacheControlCacheable() { responseCacheControl = ResponseCacheControl.builder() .setMaxAge(10) .build(); - Assertions.assertTrue(policy.isResponseCacheable(responseCacheControl, "HEAD", response)); + Assertions.assertTrue(policy.isResponseCacheable(responseCacheControl, request, response)); response = new BasicHttpResponse(HttpStatus.SC_OK, ""); response.setHeader("Date", DateUtils.formatStandardDate(Instant.now())); @@ -452,40 +365,29 @@ public void testIsHeadWithAnyCacheControlCacheable() { responseCacheControl = ResponseCacheControl.builder() .build(); - Assertions.assertTrue(policy.isResponseCacheable(responseCacheControl, "HEAD", response)); - } - - @Test - public void testIsGetWithout200Cacheable() { - HttpResponse response404 = new BasicHttpResponse(HttpStatus.SC_NOT_FOUND, ""); - - Assertions.assertFalse(policy.isResponseCacheable(responseCacheControl, "GET", response404)); - - response404 = new BasicHttpResponse(HttpStatus.SC_GATEWAY_TIMEOUT, ""); - - Assertions.assertFalse(policy.isResponseCacheable(responseCacheControl, "GET", response404)); + Assertions.assertTrue(policy.isResponseCacheable(responseCacheControl, request, response)); } @Test - public void testIsHeadWithout200Cacheable() { + public void testControlWithout200Cacheable() { HttpResponse response404 = new BasicHttpResponse(HttpStatus.SC_NOT_FOUND, ""); - Assertions.assertFalse(policy.isResponseCacheable(responseCacheControl, "HEAD", response404)); + Assertions.assertFalse(policy.isResponseCacheable(responseCacheControl, request, response404)); response404 = new BasicHttpResponse(HttpStatus.SC_GATEWAY_TIMEOUT, ""); - Assertions.assertFalse(policy.isResponseCacheable(responseCacheControl, "HEAD", response404)); + Assertions.assertFalse(policy.isResponseCacheable(responseCacheControl, request, response404)); } @Test public void testVaryStarIsNotCacheable() { response.setHeader("Vary", "*"); - Assertions.assertFalse(policy.isResponseCacheable(responseCacheControl, "GET", response)); + Assertions.assertFalse(policy.isResponseCacheable(responseCacheControl, request, response)); } @Test public void testVaryStarIsNotCacheableUsingSharedPublicCache() { - policy = new ResponseCachingPolicy(0, true, false, false, true); + policy = new ResponseCachingPolicy(true, false, false, true); request.setHeader("Authorization", StandardAuthScheme.BASIC + " QWxhZGRpbjpvcGVuIHNlc2FtZQ=="); response.setHeader("Vary", "*"); @@ -496,29 +398,14 @@ public void testVaryStarIsNotCacheableUsingSharedPublicCache() { } @Test - public void testIsGetWithVaryHeaderCacheable() { - response.addHeader("Vary", "Accept-Encoding"); - Assertions.assertTrue(policy.isResponseCacheable(responseCacheControl, "GET", response)); - } - - @Test - public void testIsHeadWithVaryHeaderCacheable() { - policy = new ResponseCachingPolicy(0, true, false, false, true); + public void testRequestWithVaryHeaderCacheable() { response.addHeader("Vary", "Accept-Encoding"); - Assertions.assertTrue(policy.isResponseCacheable(responseCacheControl, "HEAD", response)); - } - - @Test - public void testIsArbitraryMethodCacheable() { - - Assertions.assertFalse(policy.isResponseCacheable(responseCacheControl, "PUT", response)); - - Assertions.assertFalse(policy.isResponseCacheable(responseCacheControl, "huh", response)); + Assertions.assertTrue(policy.isResponseCacheable(responseCacheControl, request, response)); } @Test public void testIsArbitraryMethodCacheableUsingSharedPublicCache() { - policy = new ResponseCachingPolicy(0, true, false, false, true); + policy = new ResponseCachingPolicy(true, false, false, true); request = new HttpOptions("http://foo.example.com/"); request.setHeader("Authorization", StandardAuthScheme.BASIC + " QWxhZGRpbjpvcGVuIHNlc2FtZQ=="); @@ -534,12 +421,12 @@ public void testIsArbitraryMethodCacheableUsingSharedPublicCache() { public void testResponsesWithMultipleAgeHeadersAreCacheable() { response.addHeader("Age", "3"); response.addHeader("Age", "5"); - Assertions.assertTrue(policy.isResponseCacheable(responseCacheControl, "GET", response)); + Assertions.assertTrue(policy.isResponseCacheable(responseCacheControl, request, response)); } @Test public void testResponsesWithMultipleAgeHeadersAreNotCacheableUsingSharedPublicCache() { - policy = new ResponseCachingPolicy(0, true, false, false, true); + policy = new ResponseCachingPolicy(true, false, false, true); request.setHeader("Authorization", StandardAuthScheme.BASIC + " QWxhZGRpbjpvcGVuIHNlc2FtZQ=="); response.addHeader("Age", "3"); @@ -554,12 +441,12 @@ public void testResponsesWithMultipleAgeHeadersAreNotCacheableUsingSharedPublicC public void testResponsesWithMultipleDateHeadersAreNotCacheable() { response.addHeader("Date", DateUtils.formatStandardDate(now)); response.addHeader("Date", DateUtils.formatStandardDate(sixSecondsAgo)); - Assertions.assertFalse(policy.isResponseCacheable(responseCacheControl, "GET", response)); + Assertions.assertFalse(policy.isResponseCacheable(responseCacheControl, request, response)); } @Test public void testResponsesWithMultipleDateHeadersAreNotCacheableUsingSharedPublicCache() { - policy = new ResponseCachingPolicy(0, true, false, false, true); + policy = new ResponseCachingPolicy(true, false, false, true); request.setHeader("Authorization", StandardAuthScheme.BASIC + " QWxhZGRpbjpvcGVuIHNlc2FtZQ=="); response.addHeader("Date", DateUtils.formatStandardDate(now)); @@ -573,12 +460,12 @@ public void testResponsesWithMultipleDateHeadersAreNotCacheableUsingSharedPublic @Test public void testResponsesWithMalformedDateHeadersAreNotCacheable() { response.addHeader("Date", "garbage"); - Assertions.assertFalse(policy.isResponseCacheable(responseCacheControl, "GET", response)); + Assertions.assertFalse(policy.isResponseCacheable(responseCacheControl, request, response)); } @Test public void testResponsesWithMalformedDateHeadersAreNotCacheableUsingSharedPublicCache() { - policy = new ResponseCachingPolicy(0, true, false, false, true); + policy = new ResponseCachingPolicy(true, false, false, true); request.setHeader("Authorization", StandardAuthScheme.BASIC + " QWxhZGRpbjpvcGVuIHNlc2FtZQ=="); response.addHeader("Date", "garbage"); @@ -592,12 +479,12 @@ public void testResponsesWithMalformedDateHeadersAreNotCacheableUsingSharedPubli public void testResponsesWithMultipleExpiresHeadersAreNotCacheable() { response.addHeader("Expires", DateUtils.formatStandardDate(now)); response.addHeader("Expires", DateUtils.formatStandardDate(sixSecondsAgo)); - Assertions.assertFalse(policy.isResponseCacheable(responseCacheControl, "GET", response)); + Assertions.assertFalse(policy.isResponseCacheable(responseCacheControl, request, response)); } @Test public void testResponsesWithMultipleExpiresHeadersAreNotCacheableUsingSharedPublicCache() { - policy = new ResponseCachingPolicy(0, true, false, false, true); + policy = new ResponseCachingPolicy(true, false, false, true); request.setHeader("Authorization", StandardAuthScheme.BASIC + " QWxhZGRpbjpvcGVuIHNlc2FtZQ=="); response.addHeader("Expires", DateUtils.formatStandardDate(now)); @@ -608,28 +495,10 @@ public void testResponsesWithMultipleExpiresHeadersAreNotCacheableUsingSharedPub Assertions.assertFalse(policy.isResponseCacheable(responseCacheControl, request, response)); } - @Test - public void testResponseThatHasTooMuchContentIsNotCacheable() { - response.setHeader("Content-Length", "9000"); - Assertions.assertFalse(policy.isResponseCacheable(responseCacheControl, "GET", response)); - } - - @Test - public void testResponseThatHasTooMuchContentIsNotCacheableUsingSharedPublicCache() { - policy = new ResponseCachingPolicy(0, true, false, false, true); - - request.setHeader("Authorization", StandardAuthScheme.BASIC + " QWxhZGRpbjpvcGVuIHNlc2FtZQ=="); - response.setHeader("Content-Length", "9000"); - responseCacheControl = ResponseCacheControl.builder() - .setCachePublic(true) - .build(); - Assertions.assertFalse(policy.isResponseCacheable(responseCacheControl, request, response)); - } - @Test public void testResponsesThatAreSmallEnoughAreCacheable() { response.setHeader("Content-Length", "0"); - Assertions.assertTrue(policy.isResponseCacheable(responseCacheControl, "GET", response)); + Assertions.assertTrue(policy.isResponseCacheable(responseCacheControl, request, response)); } @Test @@ -646,14 +515,14 @@ public void testResponsesToHEADWithQueryParamsButNoExplicitCachingAreNotCacheabl @Test public void testResponsesToGETWithQueryParamsButNoExplicitCachingAreNotCacheableEvenWhen1_0QueryCachingDisabled() { - policy = new ResponseCachingPolicy(0, true, true, false, false); + policy = new ResponseCachingPolicy(true, true, false, false); request = new BasicHttpRequest("GET", "/foo?s=bar"); Assertions.assertFalse(policy.isResponseCacheable(responseCacheControl, request, response)); } @Test public void testResponsesToHEADWithQueryParamsButNoExplicitCachingAreNotCacheableEvenWhen1_0QueryCachingDisabled() { - policy = new ResponseCachingPolicy(0, true, true, false, false); + policy = new ResponseCachingPolicy(true, true, false, false); request = new BasicHttpRequest("HEAD", "/foo?s=bar"); Assertions.assertFalse(policy.isResponseCacheable(responseCacheControl, request, response)); } @@ -668,7 +537,7 @@ public void testResponsesToGETWithQueryParamsAndExplicitCachingAreCacheable() { @Test public void testResponsesToHEADWithQueryParamsAndExplicitCachingAreCacheable() { - policy = new ResponseCachingPolicy(0, true, false, false, true); + policy = new ResponseCachingPolicy(true, false, false, true); request = new BasicHttpRequest("HEAD", "/foo?s=bar"); response.setHeader("Date", DateUtils.formatStandardDate(now)); response.setHeader("Expires", DateUtils.formatStandardDate(tenSecondsFromNow)); @@ -677,7 +546,7 @@ public void testResponsesToHEADWithQueryParamsAndExplicitCachingAreCacheable() { @Test public void testResponsesToGETWithQueryParamsAndExplicitCachingAreCacheableEvenWhen1_0QueryCachingDisabled() { - policy = new ResponseCachingPolicy(0, true, true, false, true); + policy = new ResponseCachingPolicy(true, true, false, true); request = new BasicHttpRequest("GET", "/foo?s=bar"); response.setHeader("Date", DateUtils.formatStandardDate(now)); response.setHeader("Expires", DateUtils.formatStandardDate(tenSecondsFromNow)); @@ -686,7 +555,7 @@ public void testResponsesToGETWithQueryParamsAndExplicitCachingAreCacheableEvenW @Test public void testResponsesToHEADWithQueryParamsAndExplicitCachingAreCacheableEvenWhen1_0QueryCachingDisabled() { - policy = new ResponseCachingPolicy(0, true, true, false, true); + policy = new ResponseCachingPolicy(true, true, false, true); request = new BasicHttpRequest("HEAD", "/foo?s=bar"); response.setHeader("Date", DateUtils.formatStandardDate(now)); response.setHeader("Expires", DateUtils.formatStandardDate(tenSecondsFromNow)); @@ -711,7 +580,7 @@ public void headsWithQueryParametersDirectlyFrom1_0OriginsAreNotCacheable() { @Test public void getsWithQueryParametersDirectlyFrom1_0OriginsAreNotCacheableEvenWithSetting() { - policy = new ResponseCachingPolicy(0, true, true, false, true); + policy = new ResponseCachingPolicy(true, true, false, true); request = new BasicHttpRequest("GET", "/foo?s=bar"); response = new BasicHttpResponse(HttpStatus.SC_OK, "OK"); response.setVersion(HttpVersion.HTTP_1_0); @@ -720,7 +589,7 @@ public void getsWithQueryParametersDirectlyFrom1_0OriginsAreNotCacheableEvenWith @Test public void headsWithQueryParametersDirectlyFrom1_0OriginsAreNotCacheableEvenWithSetting() { - policy = new ResponseCachingPolicy(0, true, true, false, true); + policy = new ResponseCachingPolicy(true, true, false, true); request = new BasicHttpRequest("HEAD", "/foo?s=bar"); response = new BasicHttpResponse(HttpStatus.SC_OK, "OK"); response.setVersion(HttpVersion.HTTP_1_0); @@ -739,7 +608,7 @@ public void getsWithQueryParametersDirectlyFrom1_0OriginsAreCacheableWithExpires @Test public void headsWithQueryParametersDirectlyFrom1_0OriginsAreCacheableWithExpires() { - policy = new ResponseCachingPolicy(0, true, false, false, true); + policy = new ResponseCachingPolicy(true, false, false, true); request = new BasicHttpRequest("HEAD", "/foo?s=bar"); response = new BasicHttpResponse(HttpStatus.SC_OK, "OK"); response.setVersion(HttpVersion.HTTP_1_0); @@ -750,7 +619,7 @@ public void headsWithQueryParametersDirectlyFrom1_0OriginsAreCacheableWithExpire @Test public void getsWithQueryParametersDirectlyFrom1_0OriginsCanBeNotCacheableEvenWithExpires() { - policy = new ResponseCachingPolicy(0, true, true, false, true); + policy = new ResponseCachingPolicy(true, true, false, true); request = new BasicHttpRequest("GET", "/foo?s=bar"); response = new BasicHttpResponse(HttpStatus.SC_OK, "OK"); response.setVersion(HttpVersion.HTTP_1_0); @@ -761,7 +630,7 @@ public void getsWithQueryParametersDirectlyFrom1_0OriginsCanBeNotCacheableEvenWi @Test public void headsWithQueryParametersDirectlyFrom1_0OriginsCanBeNotCacheableEvenWithExpires() { - policy = new ResponseCachingPolicy(0, true, true, false, true); + policy = new ResponseCachingPolicy(true, true, false, true); request = new BasicHttpRequest("HEAD", "/foo?s=bar"); response = new BasicHttpResponse(HttpStatus.SC_OK, "OK"); response.setVersion(HttpVersion.HTTP_1_0); @@ -795,7 +664,7 @@ public void getsWithQueryParametersFrom1_0OriginsViaProxiesAreCacheableWithExpir @Test public void headsWithQueryParametersFrom1_0OriginsViaProxiesAreCacheableWithExpires() { - policy = new ResponseCachingPolicy(0, true, false, false, true); + policy = new ResponseCachingPolicy(true, false, false, true); request = new BasicHttpRequest("HEAD", "/foo?s=bar"); response.setHeader("Date", DateUtils.formatStandardDate(now)); response.setHeader("Expires", DateUtils.formatStandardDate(tenSecondsFromNow)); @@ -805,7 +674,7 @@ public void headsWithQueryParametersFrom1_0OriginsViaProxiesAreCacheableWithExpi @Test public void getsWithQueryParametersFrom1_0OriginsViaProxiesCanNotBeCacheableEvenWithExpires() { - policy = new ResponseCachingPolicy(0, true, true, true, true); + policy = new ResponseCachingPolicy(true, true, true, true); request = new BasicHttpRequest("GET", "/foo?s=bar"); response.setHeader("Date", DateUtils.formatStandardDate(now)); response.setHeader("Expires", DateUtils.formatStandardDate(tenSecondsFromNow)); @@ -815,7 +684,7 @@ public void getsWithQueryParametersFrom1_0OriginsViaProxiesCanNotBeCacheableEven @Test public void headsWithQueryParametersFrom1_0OriginsViaProxiesCanNotBeCacheableEvenWithExpires() { - policy = new ResponseCachingPolicy(0, true, true, true, true); + policy = new ResponseCachingPolicy(true, true, true, true); request = new BasicHttpRequest("HEAD", "/foo?s=bar"); response.setHeader("Date", DateUtils.formatStandardDate(now)); response.setHeader("Expires", DateUtils.formatStandardDate(tenSecondsFromNow)); @@ -834,7 +703,7 @@ public void getsWithQueryParametersFrom1_0OriginsViaExplicitProxiesAreCacheableW @Test public void headsWithQueryParametersFrom1_0OriginsViaExplicitProxiesAreCacheableWithExpires() { - policy = new ResponseCachingPolicy(0, true, false, false, false); + policy = new ResponseCachingPolicy(true, false, false, false); request = new BasicHttpRequest("HEAD", "/foo?s=bar"); response.setHeader("Date", DateUtils.formatStandardDate(now)); response.setHeader("Expires", DateUtils.formatStandardDate(tenSecondsFromNow)); @@ -844,7 +713,7 @@ public void headsWithQueryParametersFrom1_0OriginsViaExplicitProxiesAreCacheable @Test public void getsWithQueryParametersFrom1_0OriginsViaExplicitProxiesCanNotBeCacheableEvenWithExpires() { - policy = new ResponseCachingPolicy(0, true, true, true, true); + policy = new ResponseCachingPolicy(true, true, true, true); request = new BasicHttpRequest("GET", "/foo?s=bar"); response.setHeader("Date", DateUtils.formatStandardDate(now)); response.setHeader("Expires", DateUtils.formatStandardDate(tenSecondsFromNow)); @@ -854,7 +723,7 @@ public void getsWithQueryParametersFrom1_0OriginsViaExplicitProxiesCanNotBeCache @Test public void headsWithQueryParametersFrom1_0OriginsViaExplicitProxiesCanNotBeCacheableEvenWithExpires() { - policy = new ResponseCachingPolicy(0, true, true, true, true); + policy = new ResponseCachingPolicy(true, true, true, true); request = new BasicHttpRequest("HEAD", "/foo?s=bar"); response.setHeader("Date", DateUtils.formatStandardDate(now)); response.setHeader("Expires", DateUtils.formatStandardDate(tenSecondsFromNow)); @@ -875,7 +744,7 @@ public void getsWithQueryParametersFrom1_1OriginsVia1_0ProxiesAreCacheableWithEx @Test public void headsWithQueryParametersFrom1_1OriginsVia1_0ProxiesAreCacheableWithExpires() { - policy = new ResponseCachingPolicy(0, true, false, false, true); + policy = new ResponseCachingPolicy(true, false, false, true); request = new BasicHttpRequest("HEAD", "/foo?s=bar"); response = new BasicHttpResponse(HttpStatus.SC_OK, "OK"); response.setVersion(HttpVersion.HTTP_1_0); @@ -914,7 +783,7 @@ public void test302WithExplicitCachingHeaders() { public void test303WithExplicitCachingHeadersWhenPermittedByConfig() { // HTTPbis working group says ok if explicitly indicated by // response headers - policy = new ResponseCachingPolicy(0, true, false, true, true); + policy = new ResponseCachingPolicy(true, false, true, true); response.setCode(HttpStatus.SC_SEE_OTHER); response.setHeader("Date", DateUtils.formatStandardDate(now)); responseCacheControl = ResponseCacheControl.builder() @@ -955,8 +824,8 @@ void testIsResponseCacheableNullCacheControl() { // Create ResponseCachingPolicy instance and test the method - policy = new ResponseCachingPolicy(0, true, false, false, false); - request = new BasicHttpRequest("POST", "/foo"); + policy = new ResponseCachingPolicy(true, false, false, false); + request = new BasicHttpRequest("GET", "/foo"); assertTrue(policy.isResponseCacheable(responseCacheControl, request, response)); } @@ -973,8 +842,8 @@ void testIsResponseCacheableNotNullCacheControlSmaxAge60() { // Create ResponseCachingPolicy instance and test the method - policy = new ResponseCachingPolicy(0, true, false, false, false); - request = new BasicHttpRequest("POST", "/foo"); + policy = new ResponseCachingPolicy(true, false, false, false); + request = new BasicHttpRequest("GET", "/foo"); responseCacheControl = ResponseCacheControl.builder() .setMaxAge(60) .build(); @@ -993,8 +862,8 @@ void testIsResponseCacheableNotNullCacheControlMaxAge60() { // Create ResponseCachingPolicy instance and test the method - policy = new ResponseCachingPolicy(0, true, false, false,false); - request = new BasicHttpRequest("POST", "/foo"); + policy = new ResponseCachingPolicy(true, false, false,false); + request = new BasicHttpRequest("GET", "/foo"); responseCacheControl = ResponseCacheControl.builder() .setMaxAge(60) .build(); @@ -1013,8 +882,8 @@ void testIsResponseCacheableNotExsiresAndDate() { // Create ResponseCachingPolicy instance and test the method - policy = new ResponseCachingPolicy(0, true, false, false,false); - request = new BasicHttpRequest("POST", "/foo"); + policy = new ResponseCachingPolicy(true, false, false,false); + request = new BasicHttpRequest("GET", "/foo"); assertTrue(policy.isResponseCacheable(responseCacheControl, request, response)); } @@ -1029,7 +898,7 @@ public void testIsResponseCacheable() { request = new BasicHttpRequest("GET","/foo?s=bar"); // HTTPbis working group says ok if explicitly indicated by // response headers - policy = new ResponseCachingPolicy(0, true, false, true, true); + policy = new ResponseCachingPolicy(true, false, true, true); response.setCode(HttpStatus.SC_OK); response.setHeader("Date", DateUtils.formatStandardDate(now)); assertTrue(policy.isResponseCacheable(responseCacheControl, request, response)); @@ -1042,7 +911,7 @@ void testIsResponseCacheableNoCache() { response.setHeader(HttpHeaders.DATE, DateUtils.formatStandardDate(Instant.now())); // Create ResponseCachingPolicy instance and test the method - policy = new ResponseCachingPolicy(0, true, false, false, true); + policy = new ResponseCachingPolicy(true, false, false, true); request = new BasicHttpRequest("GET", "/foo"); responseCacheControl = ResponseCacheControl.builder() .setNoCache(true) @@ -1057,7 +926,7 @@ void testIsResponseCacheableNoStore() { response.setHeader(HttpHeaders.DATE, DateUtils.formatStandardDate(Instant.now())); // Create ResponseCachingPolicy instance and test the method - policy = new ResponseCachingPolicy(0, true, false, false, false); + policy = new ResponseCachingPolicy(true, false, false, false); request = new BasicHttpRequest("GET", "/foo"); responseCacheControl = ResponseCacheControl.builder() .setNoStore(true) @@ -1072,6 +941,6 @@ public void testImmutableAndFreshResponseIsCacheable() { .setMaxAge(3600) // set this to a value that ensures the response is still fresh .build(); - Assertions.assertTrue(policy.isResponseCacheable(responseCacheControl, "GET", response)); + Assertions.assertTrue(policy.isResponseCacheable(responseCacheControl, request, response)); } } \ No newline at end of file From 6c026b4ab92b132e6115d86558230ae738b8572b Mon Sep 17 00:00:00 2001 From: Oleg Kalnichevski Date: Fri, 3 Nov 2023 13:34:55 +0100 Subject: [PATCH 4/8] HTTPCLIENT-2277: Aligned CachedResponseSuitabilityChecker with the specification requirements per RFC 9111 section 4 --- .../hc/client5/http/cache/HttpCacheEntry.java | 21 ++ .../http/impl/cache/AsyncCachingExec.java | 136 +++---- .../http/impl/cache/CacheKeyGenerator.java | 84 ++++- .../http/impl/cache/CacheSuitability.java | 44 +++ .../http/impl/cache/CacheValidityPolicy.java | 67 +--- .../CachedResponseSuitabilityChecker.java | 282 ++++++++------- .../client5/http/impl/cache/CachingExec.java | 108 +++--- .../http/impl/cache/ResponseCacheControl.java | 9 +- .../impl/cache/ResponseCachingPolicy.java | 38 -- .../impl/cache/TestCacheKeyGenerator.java | 40 +++ .../impl/cache/TestCacheValidityPolicy.java | 61 ---- .../TestCachedResponseSuitabilityChecker.java | 336 ++++++++++++------ .../impl/cache/TestProtocolRequirements.java | 61 +--- 13 files changed, 732 insertions(+), 555 deletions(-) create mode 100644 httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CacheSuitability.java diff --git a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/cache/HttpCacheEntry.java b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/cache/HttpCacheEntry.java index 314cf10af7..3b734ff44a 100644 --- a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/cache/HttpCacheEntry.java +++ b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/cache/HttpCacheEntry.java @@ -342,6 +342,13 @@ public Iterator
headerIterator(final String name) { return responseHeaders.headerIterator(name); } + /** + * @since 5.3 + */ + public MessageHeaders responseHeaders() { + return responseHeaders; + } + /** * Gets the Date value of the "Date" header or null if the header is missing or cannot be * parsed. @@ -440,6 +447,13 @@ public String getRequestURI() { return requestURI; } + /** + * @since 5.3 + */ + public MessageHeaders requestHeaders() { + return requestHeaders; + } + /** * @since 5.3 */ @@ -447,6 +461,13 @@ public Iterator
requestHeaderIterator() { return requestHeaders.headerIterator(); } + /** + * @since 5.3 + */ + public Iterator
requestHeaderIterator(final String headerName) { + return requestHeaders.headerIterator(headerName); + } + /** * Tests if the given {@link HttpCacheEntry} is newer than the given {@link MessageHeaders} * by comparing values of their {@literal DATE} header. In case the given entry, or the message, 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 f30eeff1f4..fab7bff63b 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 @@ -229,6 +229,9 @@ public void execute( } final RequestCacheControl requestCacheControl = CacheControlHeaderParser.INSTANCE.parse(request); + if (LOG.isDebugEnabled()) { + LOG.debug("Request cache control: {}", requestCacheControl); + } if (cacheableRequestPolicy.isServableFromCache(requestCacheControl, request)) { operation.setDependency(responseCache.match(target, request, new FutureCallback() { @@ -242,6 +245,9 @@ public void completed(final CacheMatch result) { handleCacheMiss(requestCacheControl, root, target, request, entityProducer, scope, chain, asyncExecCallback); } else { final ResponseCacheControl responseCacheControl = CacheControlHeaderParser.INSTANCE.parse(hit.entry); + if (LOG.isDebugEnabled()) { + LOG.debug("Response cache control: {}", responseCacheControl); + } handleCacheHit(requestCacheControl, responseCacheControl, hit, target, request, entityProducer, scope, chain, asyncExecCallback); } } @@ -590,21 +596,11 @@ private void handleCacheHit( recordCacheHit(target, request); final Instant now = getCurrentDate(); - if (requestCacheControl.isNoCache()) { - // Revalidate with the server due to no-cache directive in response - if (LOG.isDebugEnabled()) { - LOG.debug("Revalidating with server due to no-cache directive in response."); - } - revalidateCacheEntry(requestCacheControl, responseCacheControl, hit, target, request, entityProducer, scope, chain, asyncExecCallback); - return; + final CacheSuitability cacheSuitability = suitabilityChecker.assessSuitability(requestCacheControl, responseCacheControl, request, hit.entry, now); + if (LOG.isDebugEnabled()) { + LOG.debug("Request {} {}: {}", request.getMethod(), request.getRequestUri(), cacheSuitability); } - - if (suitabilityChecker.canCachedResponseBeUsed(requestCacheControl, responseCacheControl, request, hit.entry, now)) { - if (responseCachingPolicy.responseContainsNoCacheDirective(responseCacheControl, hit.entry)) { - // Revalidate with the server due to no-cache directive in response - revalidateCacheEntry(requestCacheControl, responseCacheControl, hit, target, request, entityProducer, scope, chain, asyncExecCallback); - return; - } + if (cacheSuitability == CacheSuitability.FRESH || cacheSuitability == CacheSuitability.FRESH_ENOUGH) { LOG.debug("Cache hit"); try { final SimpleHttpResponse cacheResponse = generateCachedResponse(hit.entry, request, context); @@ -623,60 +619,71 @@ private void handleCacheHit( } } } - } else if (!mayCallBackend(requestCacheControl)) { - LOG.debug("Cache entry not suitable but only-if-cached requested"); - final SimpleHttpResponse cacheResponse = generateGatewayTimeout(context); - triggerResponse(cacheResponse, scope, asyncExecCallback); - } else if (!(hit.entry.getStatus() == HttpStatus.SC_NOT_MODIFIED && !suitabilityChecker.isConditional(request))) { - LOG.debug("Revalidating cache entry"); - final boolean staleIfErrorEnabled = responseCachingPolicy.isStaleIfErrorEnabled(responseCacheControl, hit.entry); - if (cacheRevalidator != null - && !staleResponseNotAllowed(requestCacheControl, responseCacheControl, hit.entry, now) - && (validityPolicy.mayReturnStaleWhileRevalidating(responseCacheControl, hit.entry, now) || staleIfErrorEnabled)) { - LOG.debug("Serving stale with asynchronous revalidation"); - try { - final SimpleHttpResponse cacheResponse = generateCachedResponse(hit.entry, request, context); - final String exchangeId = ExecSupport.getNextExchangeId(); - context.setExchangeId(exchangeId); - final AsyncExecChain.Scope fork = new AsyncExecChain.Scope( - exchangeId, - scope.route, - scope.originalRequest, - new ComplexFuture<>(null), - HttpClientContext.create(), - scope.execRuntime.fork(), - scope.scheduler, - scope.execCount); - cacheRevalidator.revalidateCacheEntry( - hit.getEntryKey(), - asyncExecCallback, - asyncExecCallback1 -> revalidateCacheEntry(requestCacheControl, responseCacheControl, - hit, target, request, entityProducer, fork, chain, asyncExecCallback1)); - triggerResponse(cacheResponse, scope, asyncExecCallback); - } catch (final ResourceIOException ex) { - if (staleIfErrorEnabled) { - if (LOG.isDebugEnabled()) { - LOG.debug("Serving stale response due to IOException and stale-if-error enabled"); - } - try { - final SimpleHttpResponse cacheResponse = generateCachedResponse(hit.entry, request, context); - triggerResponse(cacheResponse, scope, asyncExecCallback); - } catch (final ResourceIOException ex2) { + } else { + if (!mayCallBackend(requestCacheControl)) { + LOG.debug("Cache entry not is not fresh and only-if-cached requested"); + final SimpleHttpResponse cacheResponse = generateGatewayTimeout(context); + triggerResponse(cacheResponse, scope, asyncExecCallback); + } else if (cacheSuitability == CacheSuitability.MISMATCH) { + LOG.debug("Cache entry does not match the request; calling backend"); + callBackend(target, request, entityProducer, scope, chain, asyncExecCallback); + } else if (entityProducer != null && !entityProducer.isRepeatable()) { + LOG.debug("Request is not repeatable; calling backend"); + callBackend(target, request, entityProducer, scope, chain, asyncExecCallback); + } else if (hit.entry.getStatus() == HttpStatus.SC_NOT_MODIFIED && !suitabilityChecker.isConditional(request)) { + LOG.debug("Cache entry with NOT_MODIFIED does not match the non-conditional request; calling backend"); + callBackend(target, request, entityProducer, scope, chain, asyncExecCallback); + } else if (cacheSuitability == CacheSuitability.REVALIDATION_REQUIRED || cacheSuitability == CacheSuitability.STALE) { + LOG.debug("Revalidating cache entry"); + final boolean staleIfErrorEnabled = responseCachingPolicy.isStaleIfErrorEnabled(responseCacheControl, hit.entry); + if (cacheRevalidator != null + && !staleResponseNotAllowed(requestCacheControl, responseCacheControl, hit.entry, now) + && (validityPolicy.mayReturnStaleWhileRevalidating(responseCacheControl, hit.entry, now) || staleIfErrorEnabled)) { + LOG.debug("Serving stale with asynchronous revalidation"); + try { + final SimpleHttpResponse cacheResponse = generateCachedResponse(hit.entry, request, context); + final String exchangeId = ExecSupport.getNextExchangeId(); + context.setExchangeId(exchangeId); + final AsyncExecChain.Scope fork = new AsyncExecChain.Scope( + exchangeId, + scope.route, + scope.originalRequest, + new ComplexFuture<>(null), + HttpClientContext.create(), + scope.execRuntime.fork(), + scope.scheduler, + scope.execCount); + cacheRevalidator.revalidateCacheEntry( + hit.getEntryKey(), + asyncExecCallback, + asyncExecCallback1 -> revalidateCacheEntry(requestCacheControl, responseCacheControl, + hit, target, request, entityProducer, fork, chain, asyncExecCallback1)); + triggerResponse(cacheResponse, scope, asyncExecCallback); + } catch (final ResourceIOException ex) { + if (staleIfErrorEnabled) { if (LOG.isDebugEnabled()) { - LOG.debug("Failed to generate cached response, falling back to backend", ex2); + LOG.debug("Serving stale response due to IOException and stale-if-error enabled"); + } + try { + final SimpleHttpResponse cacheResponse = generateCachedResponse(hit.entry, request, context); + 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); } - callBackend(target, request, entityProducer, scope, chain, asyncExecCallback); + } else { + asyncExecCallback.failed(ex); } - } else { - asyncExecCallback.failed(ex); } + } else { + revalidateCacheEntry(requestCacheControl, responseCacheControl, hit, target, request, entityProducer, scope, chain, asyncExecCallback); } } else { - revalidateCacheEntry(requestCacheControl, responseCacheControl, hit, target, request, entityProducer, scope, chain, asyncExecCallback); + LOG.debug("Cache entry not usable; calling backend"); + callBackend(target, request, entityProducer, scope, chain, asyncExecCallback); } - } else { - LOG.debug("Cache entry not usable; calling backend"); - callBackend(target, request, entityProducer, scope, chain, asyncExecCallback); } } @@ -773,8 +780,7 @@ public AsyncDataConsumer handleResponse( final Instant responseDate1 = getCurrentDate(); final AsyncExecCallback callback1; - if (revalidationResponseIsTooOld(backendResponse1, hit.entry) - && (entityProducer == null || entityProducer.isRepeatable())) { + if (revalidationResponseIsTooOld(backendResponse1, hit.entry)) { final HttpRequest unconditional = conditionalRequestBuilder.buildUnconditionalRequest( BasicRequestBuilder.copy(scope.originalRequest).build()); @@ -880,14 +886,14 @@ private void handleCacheMiss( triggerResponse(cacheResponse, scope, asyncExecCallback); } - if (partialMatch != null && partialMatch.entry.hasVariants()) { + if (partialMatch != null && partialMatch.entry.hasVariants() && entityProducer == null) { operation.setDependency(responseCache.getVariants( partialMatch, new FutureCallback>() { @Override public void completed(final Collection variants) { - if (variants != null && !variants.isEmpty() && (entityProducer == null || entityProducer.isRepeatable())) { + if (variants != null && !variants.isEmpty()) { negotiateResponseFromVariants(target, request, entityProducer, scope, chain, asyncExecCallback, variants); } else { callBackend(target, request, entityProducer, scope, chain, asyncExecCallback); diff --git a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CacheKeyGenerator.java b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CacheKeyGenerator.java index d8b6d622f9..9e68de815a 100644 --- a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CacheKeyGenerator.java +++ b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CacheKeyGenerator.java @@ -34,7 +34,11 @@ import java.util.Iterator; import java.util.List; import java.util.Locale; +import java.util.Spliterator; +import java.util.Spliterators; import java.util.concurrent.atomic.AtomicBoolean; +import java.util.function.Consumer; +import java.util.stream.StreamSupport; import org.apache.hc.client5.http.cache.HttpCacheEntry; import org.apache.hc.core5.annotation.Contract; @@ -42,15 +46,22 @@ import org.apache.hc.core5.annotation.ThreadingBehavior; import org.apache.hc.core5.function.Resolver; import org.apache.hc.core5.http.Header; +import org.apache.hc.core5.http.HeaderElement; import org.apache.hc.core5.http.HttpHeaders; import org.apache.hc.core5.http.HttpHost; import org.apache.hc.core5.http.HttpRequest; import org.apache.hc.core5.http.MessageHeaders; +import org.apache.hc.core5.http.NameValuePair; import org.apache.hc.core5.http.URIScheme; +import org.apache.hc.core5.http.message.BasicHeaderElementIterator; +import org.apache.hc.core5.http.message.BasicHeaderValueFormatter; +import org.apache.hc.core5.http.message.BasicNameValuePair; import org.apache.hc.core5.net.PercentCodec; import org.apache.hc.core5.net.URIAuthority; import org.apache.hc.core5.net.URIBuilder; import org.apache.hc.core5.util.Args; +import org.apache.hc.core5.util.CharArrayBuffer; +import org.apache.hc.core5.util.TextUtils; /** * @since 4.1 @@ -201,6 +212,56 @@ public static List variantNames(final MessageHeaders message) { return names; } + @Internal + public static void normalizeElements(final MessageHeaders message, final String headerName, final Consumer consumer) { + // User-Agent as a special case due to its grammar + if (headerName.equalsIgnoreCase(HttpHeaders.USER_AGENT)) { + final Header header = message.getFirstHeader(headerName); + if (header != null) { + consumer.accept(header.getValue().toLowerCase(Locale.ROOT)); + } + } else { + normalizeElements(message.headerIterator(headerName), consumer); + } + } + + @Internal + public static void normalizeElements(final Iterator
iterator, final Consumer consumer) { + final Iterator it = new BasicHeaderElementIterator(iterator); + StreamSupport.stream(Spliterators.spliteratorUnknownSize(it, Spliterator.NONNULL), false) + .filter(e -> !TextUtils.isBlank(e.getName())) + .map(e -> { + if (e.getValue() == null && e.getParameterCount() == 0) { + return e.getName().toLowerCase(Locale.ROOT); + } else { + final CharArrayBuffer buf = new CharArrayBuffer(1024); + BasicHeaderValueFormatter.INSTANCE.formatNameValuePair( + buf, + new BasicNameValuePair( + e.getName().toLowerCase(Locale.ROOT), + !TextUtils.isBlank(e.getValue()) ? e.getValue() : null), + false); + if (e.getParameterCount() > 0) { + for (final NameValuePair nvp : e.getParameters()) { + if (!TextUtils.isBlank(nvp.getName())) { + buf.append(';'); + BasicHeaderValueFormatter.INSTANCE.formatNameValuePair( + buf, + new BasicNameValuePair( + nvp.getName().toLowerCase(Locale.ROOT), + !TextUtils.isBlank(nvp.getValue()) ? nvp.getValue() : null), + false); + } + } + } + return buf.toString(); + } + }) + .sorted() + .distinct() + .forEach(consumer); + } + /** * Computes a "variant key" for the given request and the given variants. * @param request originating request @@ -222,24 +283,13 @@ public String generateVariantKey(final HttpRequest request, final Collection { + if (!firstToken.compareAndSet(false, true)) { + buf.append(","); + } + buf.append(PercentCodec.encode(t, StandardCharsets.UTF_8)); + }); }); buf.append("}"); return buf.toString(); diff --git a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CacheSuitability.java b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CacheSuitability.java new file mode 100644 index 0000000000..2431489414 --- /dev/null +++ b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CacheSuitability.java @@ -0,0 +1,44 @@ +/* + * ==================================================================== + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + * ==================================================================== + * + * This software consists of voluntary contributions made by many + * individuals on behalf of the Apache Software Foundation. For more + * information on the Apache Software Foundation, please see + * . + * + */ + +package org.apache.hc.client5.http.impl.cache; + +/** + * @since 5.3 + */ +enum CacheSuitability { + + MISMATCH, // the cache entry does not match the request properties and cannot be used + // to satisfy the request + FRESH, // the cache entry is fresh and can be used to satisfy the request + FRESH_ENOUGH, // the cache entry is deemed fresh enough and can be used to satisfy the request + STALE, // the cache entry is stale and may be unsuitable to satisfy the request + REVALIDATION_REQUIRED + // the cache entry is stale and must not be used to satisfy the request + // without revalidation + +} \ No newline at end of file diff --git a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CacheValidityPolicy.java b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CacheValidityPolicy.java index a79214d519..ca0dbb4635 100644 --- a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CacheValidityPolicy.java +++ b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CacheValidityPolicy.java @@ -41,6 +41,7 @@ class CacheValidityPolicy { private static final Logger LOG = LoggerFactory.getLogger(CacheValidityPolicy.class); + private final boolean useHeuristicCaching; private final float heuristicCoefficient; private final TimeValue heuristicDefaultLifetime; @@ -53,6 +54,7 @@ class CacheValidityPolicy { */ CacheValidityPolicy(final CacheConfig config) { super(); + this.useHeuristicCaching = config != null ? config.isHeuristicCachingEnabled() : CacheConfig.DEFAULT.isHeuristicCachingEnabled(); this.heuristicCoefficient = config != null ? config.getHeuristicCoefficient() : CacheConfig.DEFAULT.getHeuristicCoefficient(); this.heuristicDefaultLifetime = config != null ? config.getHeuristicDefaultLifetime() : CacheConfig.DEFAULT.getHeuristicDefaultLifetime(); } @@ -117,35 +119,18 @@ public TimeValue getFreshnessLifetime(final ResponseCacheControl responseCacheCo } } - // No explicit expiration time is present in the response. A heuristic freshness lifetime might be applicable - if (LOG.isDebugEnabled()) { - LOG.debug("No explicit expiration time present in the response. Using heuristic freshness lifetime calculation."); + if (useHeuristicCaching) { + // No explicit expiration time is present in the response. A heuristic freshness lifetime might be applicable + if (LOG.isDebugEnabled()) { + LOG.debug("No explicit expiration time present in the response. Using heuristic freshness lifetime calculation."); + } + return getHeuristicFreshnessLifetime(entry); + } else { + return TimeValue.ZERO_MILLISECONDS; } - return getHeuristicFreshnessLifetime(entry); - } - - public boolean isResponseFresh(final ResponseCacheControl responseCacheControl, final HttpCacheEntry entry, - final Instant now) { - return getCurrentAge(entry, now).compareTo(getFreshnessLifetime(responseCacheControl, entry)) == -1; - } - - /** - * Decides if this response is fresh enough based Last-Modified and Date, if available. - * This entry is meant to be used when isResponseFresh returns false. - * - * The algorithm is as follows: - * if last-modified and date are defined, freshness lifetime is coefficient*(date-lastModified), - * else freshness lifetime is defaultLifetime - * - * @param entry the cache entry - * @param now what time is it currently (When is right NOW) - * @return {@code true} if the response is fresh - */ - public boolean isResponseHeuristicallyFresh(final HttpCacheEntry entry, final Instant now) { - return getCurrentAge(entry, now).compareTo(getHeuristicFreshnessLifetime(entry)) == -1; } - public TimeValue getHeuristicFreshnessLifetime(final HttpCacheEntry entry) { + TimeValue getHeuristicFreshnessLifetime(final HttpCacheEntry entry) { final Instant dateValue = entry.getInstant(); final Instant lastModifiedValue = entry.getLastModified(); @@ -189,7 +174,7 @@ private boolean mayReturnStaleIfError(final CacheControl responseCacheControl, f staleness.compareTo(TimeValue.ofSeconds(responseCacheControl.getStaleIfError())) <= 0; } - protected TimeValue getApparentAge(final HttpCacheEntry entry) { + TimeValue getApparentAge(final HttpCacheEntry entry) { final Instant dateValue = entry.getInstant(); if (dateValue == null) { return CacheSupport.MAX_AGE; @@ -216,7 +201,7 @@ protected TimeValue getApparentAge(final HttpCacheEntry entry) { * is negative. If the Age value is invalid (cannot be parsed into a number or contains non-numeric characters), * this method returns 0. */ - protected long getAgeValue(final HttpCacheEntry entry) { + long getAgeValue(final HttpCacheEntry entry) { final Header age = entry.getFirstHeader(HttpHeaders.AGE); if (age != null) { final AtomicReference firstToken = new AtomicReference<>(); @@ -231,44 +216,29 @@ protected long getAgeValue(final HttpCacheEntry entry) { return 0; } - - - protected TimeValue getCorrectedAgeValue(final HttpCacheEntry entry) { + TimeValue getCorrectedAgeValue(final HttpCacheEntry entry) { final long ageValue = getAgeValue(entry); final long responseDelay = getResponseDelay(entry).toSeconds(); return TimeValue.ofSeconds(ageValue + responseDelay); } - protected TimeValue getResponseDelay(final HttpCacheEntry entry) { + TimeValue getResponseDelay(final HttpCacheEntry entry) { final Duration diff = Duration.between(entry.getRequestInstant(), entry.getResponseInstant()); return TimeValue.ofSeconds(diff.getSeconds()); } - protected TimeValue getCorrectedInitialAge(final HttpCacheEntry entry) { + TimeValue getCorrectedInitialAge(final HttpCacheEntry entry) { final long apparentAge = getApparentAge(entry).toSeconds(); final long correctedReceivedAge = getCorrectedAgeValue(entry).toSeconds(); return TimeValue.ofSeconds(Math.max(apparentAge, correctedReceivedAge)); } - protected TimeValue getResidentTime(final HttpCacheEntry entry, final Instant now) { + TimeValue getResidentTime(final HttpCacheEntry entry, final Instant now) { final Duration diff = Duration.between(entry.getResponseInstant(), now); return TimeValue.ofSeconds(diff.getSeconds()); } - - protected long getMaxAge(final ResponseCacheControl responseCacheControl) { - final long maxAge = responseCacheControl.getMaxAge(); - final long sharedMaxAge = responseCacheControl.getSharedMaxAge(); - if (sharedMaxAge == -1) { - return maxAge; - } else if (maxAge == -1) { - return sharedMaxAge; - } else { - return Math.min(maxAge, sharedMaxAge); - } - } - - public TimeValue getStaleness(final ResponseCacheControl responseCacheControl, final HttpCacheEntry entry, final Instant now) { + TimeValue getStaleness(final ResponseCacheControl responseCacheControl, final HttpCacheEntry entry, final Instant now) { final TimeValue age = getCurrentAge(entry, now); final TimeValue freshness = getFreshnessLifetime(responseCacheControl, entry); if (age.compareTo(freshness) <= 0) { @@ -277,5 +247,4 @@ public TimeValue getStaleness(final ResponseCacheControl responseCacheControl, f return TimeValue.ofSeconds(age.toSeconds() - freshness.toSeconds()); } - } diff --git a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachedResponseSuitabilityChecker.java b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachedResponseSuitabilityChecker.java index 0e1161b039..69a053540b 100644 --- a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachedResponseSuitabilityChecker.java +++ b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachedResponseSuitabilityChecker.java @@ -26,8 +26,16 @@ */ package org.apache.hc.client5.http.impl.cache; +import java.net.URI; +import java.net.URISyntaxException; import java.time.Instant; +import java.util.ArrayList; +import java.util.HashSet; import java.util.Iterator; +import java.util.List; +import java.util.Locale; +import java.util.Objects; +import java.util.Set; import org.apache.hc.client5.http.cache.HttpCacheEntry; import org.apache.hc.client5.http.utils.DateUtils; @@ -51,7 +59,6 @@ class CachedResponseSuitabilityChecker { private static final Logger LOG = LoggerFactory.getLogger(CachedResponseSuitabilityChecker.class); private final boolean sharedCache; - private final boolean useHeuristicCaching; private final CacheValidityPolicy validityStrategy; CachedResponseSuitabilityChecker(final CacheValidityPolicy validityStrategy, @@ -59,140 +66,192 @@ class CachedResponseSuitabilityChecker { super(); this.validityStrategy = validityStrategy; this.sharedCache = config.isSharedCache(); - this.useHeuristicCaching = config.isHeuristicCachingEnabled(); } CachedResponseSuitabilityChecker(final CacheConfig config) { this(new CacheValidityPolicy(config), config); } - private boolean isFreshEnough(final RequestCacheControl requestCacheControl, - final ResponseCacheControl responseCacheControl, final HttpCacheEntry entry, - final Instant now) { - if (validityStrategy.isResponseFresh(responseCacheControl, entry, now)) { - return true; - } - if (useHeuristicCaching && - validityStrategy.isResponseHeuristicallyFresh(entry, now)) { - return true; - } - if (originInsistsOnFreshness(responseCacheControl)) { - return false; - } - if (requestCacheControl.getMaxStale() == -1) { - return false; + /** + * Determine if I can utilize the given {@link HttpCacheEntry} to respond to the given + * {@link HttpRequest}. + * + * @since 5.3 + */ + public CacheSuitability assessSuitability(final RequestCacheControl requestCacheControl, + final ResponseCacheControl responseCacheControl, + final HttpRequest request, + final HttpCacheEntry entry, + final Instant now) { + if (!requestMethodMatch(request, entry)) { + LOG.debug("Request method and the cache entry method do not match"); + return CacheSuitability.MISMATCH; } - return (requestCacheControl.getMaxStale() > validityStrategy.getStaleness(responseCacheControl, entry, now).toSeconds()); - } - private boolean originInsistsOnFreshness(final ResponseCacheControl responseCacheControl) { - if (responseCacheControl.isMustRevalidate()) { - return true; + if (!requestUriMatch(request, entry)) { + LOG.debug("Target request URI and the cache entry request URI do not match"); + return CacheSuitability.MISMATCH; } - if (!sharedCache) { - return false; + + if (!requestHeadersMatch(request, entry)) { + LOG.debug("Request headers nominated by the cached response do not match those of the request associated with the cache entry"); + return CacheSuitability.MISMATCH; } - return responseCacheControl.isProxyRevalidate() || responseCacheControl.getSharedMaxAge() >= 0; - } - /** - * Determine if I can utilize a {@link HttpCacheEntry} to respond to the given - * {@link HttpRequest} - * @since 5.3 - */ - public boolean canCachedResponseBeUsed(final RequestCacheControl requestCacheControl, - final ResponseCacheControl responseCacheControl, final HttpRequest request, - final HttpCacheEntry entry, final Instant now) { + if (!requestHeadersMatch(request, entry)) { + LOG.debug("Request headers nominated by the cached response do not match those of the request associated with the cache entry"); + return CacheSuitability.MISMATCH; + } - if (isGetRequestWithHeadCacheEntry(request, entry)) { - LOG.debug("Cache entry created by HEAD request cannot be used to serve GET request"); - return false; + if (requestCacheControl.isNoCache()) { + LOG.debug("Request contained no-cache directive; the cache entry must be re-validated"); + return CacheSuitability.REVALIDATION_REQUIRED; } - if (!isFreshEnough(requestCacheControl, responseCacheControl, entry, now)) { - LOG.debug("Cache entry is not fresh enough"); - return false; + if (isResponseNoCache(responseCacheControl, entry)) { + LOG.debug("Response contained no-cache directive; the cache entry must be re-validated"); + return CacheSuitability.REVALIDATION_REQUIRED; } if (hasUnsupportedConditionalHeaders(request)) { - LOG.debug("Request contains unsupported conditional headers"); - return false; + LOG.debug("Response from cache is not suitable due to the request containing unsupported conditional headers"); + return CacheSuitability.REVALIDATION_REQUIRED; } if (!isConditional(request) && entry.getStatus() == HttpStatus.SC_NOT_MODIFIED) { LOG.debug("Unconditional request and non-modified cached response"); - return false; + return CacheSuitability.REVALIDATION_REQUIRED; } - if (isConditional(request) && !allConditionalsMatch(request, entry, now)) { - LOG.debug("Conditional request and with mismatched conditions"); - return false; + if (!allConditionalsMatch(request, entry, now)) { + LOG.debug("Response from cache is not suitable due to the conditional request and with mismatched conditions"); + return CacheSuitability.REVALIDATION_REQUIRED; } - if (hasUnsupportedCacheEntryForGet(request, entry)) { - LOG.debug("HEAD response caching enabled but the cache entry does not contain a " + - "request method, entity or a 204 response"); - return false; - } - if (requestCacheControl.isNoCache()) { - LOG.debug("Response contained NO CACHE directive, cache was not suitable"); - return false; + final TimeValue currentAge = validityStrategy.getCurrentAge(entry, now); + final TimeValue freshnessLifetime = validityStrategy.getFreshnessLifetime(responseCacheControl, entry); + + final boolean fresh = currentAge.compareTo(freshnessLifetime) < 0; + + if (!fresh && responseCacheControl.isMustRevalidate()) { + LOG.debug("Response from cache is not suitable due to the response must-revalidate requirement"); + return CacheSuitability.REVALIDATION_REQUIRED; } - if (requestCacheControl.isNoStore()) { - LOG.debug("Response contained NO STORE directive, cache was not suitable"); - return false; + if (!fresh && sharedCache && responseCacheControl.isProxyRevalidate()) { + LOG.debug("Response from cache is not suitable due to the response proxy-revalidate requirement"); + return CacheSuitability.REVALIDATION_REQUIRED; } - if (requestCacheControl.getMaxAge() >= 0) { - if (validityStrategy.getCurrentAge(entry, now).toSeconds() > requestCacheControl.getMaxAge()) { - LOG.debug("Response from cache was not suitable due to max age"); - return false; + if (fresh && requestCacheControl.getMaxAge() >= 0) { + if (currentAge.toSeconds() > requestCacheControl.getMaxAge() && requestCacheControl.getMaxStale() == -1) { + LOG.debug("Response from cache is not suitable due to the request max-age requirement"); + return CacheSuitability.REVALIDATION_REQUIRED; } } - if (requestCacheControl.getMaxStale() >= 0) { - if (validityStrategy.getFreshnessLifetime(responseCacheControl, entry).toSeconds() > requestCacheControl.getMaxStale()) { - LOG.debug("Response from cache was not suitable due to max stale freshness"); - return false; + if (fresh && requestCacheControl.getMinFresh() >= 0) { + if (requestCacheControl.getMinFresh() == 0 || + freshnessLifetime.toSeconds() - currentAge.toSeconds() < requestCacheControl.getMinFresh()) { + LOG.debug("Response from cache is not suitable due to the request min-fresh requirement"); + return CacheSuitability.REVALIDATION_REQUIRED; } } - if (requestCacheControl.getMinFresh() >= 0) { - if (requestCacheControl.getMinFresh() == 0) { - return false; - } - final TimeValue age = validityStrategy.getCurrentAge(entry, now); - final TimeValue freshness = validityStrategy.getFreshnessLifetime(responseCacheControl, entry); - if (freshness.toSeconds() - age.toSeconds() < requestCacheControl.getMinFresh()) { - LOG.debug("Response from cache was not suitable due to min fresh " + - "freshness requirement"); - return false; + if (requestCacheControl.getMaxStale() >= 0) { + final long stale = currentAge.compareTo(freshnessLifetime) > 0 ? currentAge.toSeconds() - freshnessLifetime.toSeconds() : 0; + if (stale >= requestCacheControl.getMaxStale()) { + LOG.debug("Response from cache is not suitable due to the request max-stale requirement"); + return CacheSuitability.REVALIDATION_REQUIRED; + } else { + LOG.debug("The cache entry is fresh enough"); + return CacheSuitability.FRESH_ENOUGH; } } - LOG.debug("Response from cache was suitable"); - return true; - } - - private boolean isGet(final HttpRequest request) { - return Method.GET.isSame(request.getMethod()); + if (fresh) { + LOG.debug("The cache entry is fresh"); + return CacheSuitability.FRESH; + } else { + LOG.debug("The cache entry is stale"); + return CacheSuitability.STALE; + } } - private boolean isHead(final HttpRequest request) { - return Method.HEAD.isSame(request.getMethod()); + boolean requestMethodMatch(final HttpRequest request, final HttpCacheEntry entry) { + return request.getMethod().equalsIgnoreCase(entry.getRequestMethod()) || + (Method.HEAD.isSame(request.getMethod()) && Method.GET.isSame(entry.getRequestMethod())); } - private boolean entryIsNotA204Response(final HttpCacheEntry entry) { - return entry.getStatus() != HttpStatus.SC_NO_CONTENT; + boolean requestUriMatch(final HttpRequest request, final HttpCacheEntry entry) { + try { + final URI requestURI = CacheKeyGenerator.normalize(request.getUri()); + final URI cacheURI = new URI(entry.getRequestURI()); + if (requestURI.isAbsolute()) { + return Objects.equals(requestURI, cacheURI); + } else { + return Objects.equals(requestURI.getPath(), cacheURI.getPath()) && Objects.equals(requestURI.getQuery(), cacheURI.getQuery()); + } + } catch (final URISyntaxException ex) { + return false; + } } - private boolean cacheEntryDoesNotContainMethodAndEntity(final HttpCacheEntry entry) { - return entry.getRequestMethod() == null && entry.getResource() == null; + boolean requestHeadersMatch(final HttpRequest request, final HttpCacheEntry entry) { + final Iterator
it = entry.headerIterator(HttpHeaders.VARY); + if (it.hasNext()) { + final Set headerNames = new HashSet<>(); + while (it.hasNext()) { + final Header header = it.next(); + CacheSupport.parseTokens(header, e -> { + headerNames.add(e.toLowerCase(Locale.ROOT)); + }); + } + final List tokensInRequest = new ArrayList<>(); + final List tokensInCache = new ArrayList<>(); + for (final String headerName: headerNames) { + if (headerName.equalsIgnoreCase("*")) { + return false; + } + CacheKeyGenerator.normalizeElements(request, headerName, tokensInRequest::add); + CacheKeyGenerator.normalizeElements(entry.requestHeaders(), headerName, tokensInCache::add); + if (!Objects.equals(tokensInRequest, tokensInCache)) { + return false; + } + } + } + return true; } - private boolean hasUnsupportedCacheEntryForGet(final HttpRequest request, final HttpCacheEntry entry) { - return isGet(request) && cacheEntryDoesNotContainMethodAndEntity(entry) && entryIsNotA204Response(entry); + /** + * Determines if the given {@link HttpCacheEntry} requires revalidation based on the presence of the {@code no-cache} directive + * in the Cache-Control header. + *

+ * The method returns true in the following cases: + * - If the {@code no-cache} directive is present without any field names (unqualified). + * - If the {@code no-cache} directive is present with field names, and at least one of these field names is present + * in the headers of the {@link HttpCacheEntry}. + *

+ * If the {@code no-cache} directive is not present in the Cache-Control header, the method returns {@code false}. + */ + boolean isResponseNoCache(final ResponseCacheControl responseCacheControl, final HttpCacheEntry entry) { + // If no-cache directive is present and has no field names + if (responseCacheControl.isNoCache()) { + final Set noCacheFields = responseCacheControl.getNoCacheFields(); + if (noCacheFields.isEmpty()) { + LOG.debug("Revalidation required due to unqualified no-cache directive"); + return true; + } + for (final String field : noCacheFields) { + if (entry.containsHeader(field)) { + if (LOG.isDebugEnabled()) { + LOG.debug("Revalidation required due to no-cache directive with field {}", field); + } + return true; + } + } + } + return false; } /** @@ -204,22 +263,6 @@ public boolean isConditional(final HttpRequest request) { return hasSupportedEtagValidator(request) || hasSupportedLastModifiedValidator(request); } - /** - * Determines whether the given request is a {@link org.apache.hc.core5.http.Method#GET} request and the - * associated cache entry was created by a {@link org.apache.hc.core5.http.Method#HEAD} request. - * - * @param request The {@link HttpRequest} to check if it is a {@link org.apache.hc.core5.http.Method#GET} request. - * @param entry The {@link HttpCacheEntry} to check if it was created by - * a {@link org.apache.hc.core5.http.Method#HEAD} request. - * @return true if the request is a {@link org.apache.hc.core5.http.Method#GET} request and the cache entry was - * created by a {@link org.apache.hc.core5.http.Method#HEAD} request, otherwise {@code false}. - * @since 5.3 - */ - public boolean isGetRequestWithHeadCacheEntry(final HttpRequest request, final HttpCacheEntry entry) { - return isGet(request) && Method.HEAD.isSame(entry.getRequestMethod()); - } - - /** * Check that conditionals that are part of this request match * @param request The current httpRequest being made @@ -231,6 +274,10 @@ public boolean allConditionalsMatch(final HttpRequest request, final HttpCacheEn final boolean hasEtagValidator = hasSupportedEtagValidator(request); final boolean hasLastModifiedValidator = hasSupportedLastModifiedValidator(request); + if (!hasEtagValidator && !hasLastModifiedValidator) { + return true; + } + final boolean etagValidatorMatches = (hasEtagValidator) && etagValidatorMatches(request, entry); final boolean lastModifiedValidatorMatches = (hasLastModifiedValidator) && lastModifiedValidatorMatches(request, entry, now); @@ -244,18 +291,18 @@ public boolean allConditionalsMatch(final HttpRequest request, final HttpCacheEn return !hasLastModifiedValidator || lastModifiedValidatorMatches; } - private boolean hasUnsupportedConditionalHeaders(final HttpRequest request) { - return (request.getFirstHeader(HttpHeaders.IF_RANGE) != null - || request.getFirstHeader(HttpHeaders.IF_MATCH) != null - || hasValidDateField(request, HttpHeaders.IF_UNMODIFIED_SINCE)); + boolean hasUnsupportedConditionalHeaders(final HttpRequest request) { + return (request.containsHeader(HttpHeaders.IF_RANGE) + || request.containsHeader(HttpHeaders.IF_MATCH) + || request.containsHeader(HttpHeaders.IF_UNMODIFIED_SINCE)); } - private boolean hasSupportedEtagValidator(final HttpRequest request) { + boolean hasSupportedEtagValidator(final HttpRequest request) { return request.containsHeader(HttpHeaders.IF_NONE_MATCH); } - private boolean hasSupportedLastModifiedValidator(final HttpRequest request) { - return hasValidDateField(request, HttpHeaders.IF_MODIFIED_SINCE); + boolean hasSupportedLastModifiedValidator(final HttpRequest request) { + return request.containsHeader(HttpHeaders.IF_MODIFIED_SINCE); } /** @@ -264,7 +311,7 @@ private boolean hasSupportedLastModifiedValidator(final HttpRequest request) { * @param entry the cache entry * @return boolean does the etag validator match */ - private boolean etagValidatorMatches(final HttpRequest request, final HttpCacheEntry entry) { + boolean etagValidatorMatches(final HttpRequest request, final HttpCacheEntry entry) { final Header etagHeader = entry.getFirstHeader(HttpHeaders.ETAG); final String etag = (etagHeader != null) ? etagHeader.getValue() : null; final Iterator it = MessageSupport.iterate(request, HttpHeaders.IF_NONE_MATCH); @@ -285,7 +332,7 @@ private boolean etagValidatorMatches(final HttpRequest request, final HttpCacheE * @param now right NOW in time * @return boolean Does the last modified header match */ - private boolean lastModifiedValidatorMatches(final HttpRequest request, final HttpCacheEntry entry, final Instant now) { + boolean lastModifiedValidatorMatches(final HttpRequest request, final HttpCacheEntry entry, final Instant now) { final Instant lastModified = entry.getLastModified(); if (lastModified == null) { return false; @@ -302,11 +349,4 @@ private boolean lastModifiedValidatorMatches(final HttpRequest request, final Ht return true; } - private boolean hasValidDateField(final HttpRequest request, final String headerName) { - for(final Header h : request.getHeaders(headerName)) { - final Instant instant = DateUtils.parseStandardDate(h.getValue()); - return instant != null; - } - return false; - } } \ No newline at end of file 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 6311d8865f..6348033a57 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 @@ -168,6 +168,9 @@ public ClassicHttpResponse execute( final CacheHit root = result != null ? result.root : null; final RequestCacheControl requestCacheControl = CacheControlHeaderParser.INSTANCE.parse(request); + if (LOG.isDebugEnabled()) { + LOG.debug("Request cache control: {}", requestCacheControl); + } if (!cacheableRequestPolicy.isServableFromCache(requestCacheControl, request)) { LOG.debug("Request is not servable from cache"); return callBackend(target, request, scope, chain); @@ -179,6 +182,9 @@ public ClassicHttpResponse execute( return handleCacheMiss(requestCacheControl, root, target, request, scope, chain); } else { final ResponseCacheControl responseCacheControl = CacheControlHeaderParser.INSTANCE.parse(hit.entry); + if (LOG.isDebugEnabled()) { + LOG.debug("Response cache control: {}", responseCacheControl); + } return handleCacheHit(requestCacheControl, responseCacheControl, hit, target, request, scope, chain); } } @@ -238,19 +244,11 @@ private ClassicHttpResponse handleCacheHit( recordCacheHit(target, request); final Instant now = getCurrentDate(); - if (requestCacheControl.isNoCache()) { - // Revalidate with the server - return revalidateCacheEntry(requestCacheControl, responseCacheControl, hit, target, request, scope, chain); + final CacheSuitability cacheSuitability = suitabilityChecker.assessSuitability(requestCacheControl, responseCacheControl, request, hit.entry, now); + if (LOG.isDebugEnabled()) { + LOG.debug("Request {} {}: {}", request.getMethod(), request.getRequestUri(), cacheSuitability); } - - if (suitabilityChecker.canCachedResponseBeUsed(requestCacheControl, responseCacheControl, request, hit.entry, now)) { - if (responseCachingPolicy.responseContainsNoCacheDirective(responseCacheControl, hit.entry)) { - // Revalidate with the server due to no-cache directive in response - if (LOG.isDebugEnabled()) { - LOG.debug("Revalidating with server due to no-cache directive in response."); - } - return revalidateCacheEntry(requestCacheControl, responseCacheControl, hit, target, request, scope, chain); - } + if (cacheSuitability == CacheSuitability.FRESH || cacheSuitability == CacheSuitability.FRESH_ENOUGH) { LOG.debug("Cache hit"); try { return convert(generateCachedResponse(hit.entry, request, context), scope); @@ -262,44 +260,55 @@ private ClassicHttpResponse handleCacheHit( setResponseStatus(scope.clientContext, CacheResponseStatus.FAILURE); return chain.proceed(request, scope); } - } else if (!mayCallBackend(requestCacheControl)) { - LOG.debug("Cache entry not suitable but only-if-cached requested"); - return convert(generateGatewayTimeout(context), scope); - } else if (!(hit.entry.getStatus() == HttpStatus.SC_NOT_MODIFIED && !suitabilityChecker.isConditional(request))) { - LOG.debug("Revalidating cache entry"); - final boolean staleIfErrorEnabled = responseCachingPolicy.isStaleIfErrorEnabled(responseCacheControl, hit.entry); - try { - if (cacheRevalidator != null - && !staleResponseNotAllowed(requestCacheControl, responseCacheControl, hit.entry, now) - && (validityPolicy.mayReturnStaleWhileRevalidating(responseCacheControl, hit.entry, now) || staleIfErrorEnabled)) { - LOG.debug("Serving stale with asynchronous revalidation"); - final String exchangeId = ExecSupport.getNextExchangeId(); - context.setExchangeId(exchangeId); - final ExecChain.Scope fork = new ExecChain.Scope( - exchangeId, - scope.route, - scope.originalRequest, - scope.execRuntime.fork(null), - HttpClientContext.create()); - final SimpleHttpResponse response = generateCachedResponse(hit.entry, request, context); - cacheRevalidator.revalidateCacheEntry( - hit.getEntryKey(), - () -> revalidateCacheEntry(requestCacheControl, responseCacheControl, hit, target, request, fork, chain)); - return convert(response, scope); - } - return revalidateCacheEntry(requestCacheControl, responseCacheControl, hit, target, request, scope, chain); - } catch (final IOException ioex) { - if (staleIfErrorEnabled) { - if (LOG.isDebugEnabled()) { - LOG.debug("Serving stale response due to IOException and stale-if-error enabled"); + } else { + if (!mayCallBackend(requestCacheControl)) { + LOG.debug("Cache entry not is not fresh and only-if-cached requested"); + return convert(generateGatewayTimeout(context), scope); + } else if (cacheSuitability == CacheSuitability.MISMATCH) { + LOG.debug("Cache entry does not match the request; calling backend"); + return callBackend(target, request, scope, chain); + } else if (request.getEntity() != null && !request.getEntity().isRepeatable()) { + LOG.debug("Request is not repeatable; calling backend"); + return callBackend(target, request, scope, chain); + } else if (hit.entry.getStatus() == HttpStatus.SC_NOT_MODIFIED && !suitabilityChecker.isConditional(request)) { + LOG.debug("Cache entry with NOT_MODIFIED does not match the non-conditional request; calling backend"); + return callBackend(target, request, scope, chain); + } else if (cacheSuitability == CacheSuitability.REVALIDATION_REQUIRED || cacheSuitability == CacheSuitability.STALE) { + LOG.debug("Revalidating cache entry"); + final boolean staleIfErrorEnabled = responseCachingPolicy.isStaleIfErrorEnabled(responseCacheControl, hit.entry); + try { + if (cacheRevalidator != null + && !staleResponseNotAllowed(requestCacheControl, responseCacheControl, hit.entry, now) + && (validityPolicy.mayReturnStaleWhileRevalidating(responseCacheControl, hit.entry, now) || staleIfErrorEnabled)) { + LOG.debug("Serving stale with asynchronous revalidation"); + final String exchangeId = ExecSupport.getNextExchangeId(); + context.setExchangeId(exchangeId); + final ExecChain.Scope fork = new ExecChain.Scope( + exchangeId, + scope.route, + scope.originalRequest, + scope.execRuntime.fork(null), + HttpClientContext.create()); + final SimpleHttpResponse response = generateCachedResponse(hit.entry, request, context); + cacheRevalidator.revalidateCacheEntry( + hit.getEntryKey(), + () -> revalidateCacheEntry(requestCacheControl, responseCacheControl, hit, target, request, fork, chain)); + return convert(response, scope); + } + return revalidateCacheEntry(requestCacheControl, responseCacheControl, hit, target, request, scope, chain); + } 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(hit.entry, request, context), scope); } - return convert(generateCachedResponse(hit.entry, request, context), scope); + return convert(handleRevalidationFailure(requestCacheControl, responseCacheControl, hit.entry, request, context, now), scope); } - return convert(handleRevalidationFailure(requestCacheControl, responseCacheControl, hit.entry, request, context, now), scope); + } else { + LOG.debug("Cache entry not usable; calling backend"); + return callBackend(target, request, scope, chain); } - } else { - LOG.debug("Cache entry not usable; calling backend"); - return callBackend(target, request, scope, chain); } } @@ -458,7 +467,7 @@ private ClassicHttpResponse handleCacheMiss( if (!mayCallBackend(requestCacheControl)) { return new BasicClassicHttpResponse(HttpStatus.SC_GATEWAY_TIMEOUT, "Gateway Timeout"); } - if (partialMatch != null && partialMatch.entry.hasVariants()) { + if (partialMatch != null && partialMatch.entry.hasVariants() && request.getEntity() == null) { final List variants = responseCache.getVariants(partialMatch); if (variants != null && !variants.isEmpty()) { return negotiateResponseFromVariants(target, request, scope, chain, variants); @@ -509,8 +518,7 @@ ClassicHttpResponse negotiateResponseFromVariants( return callBackend(target, request, scope, chain); } - if (revalidationResponseIsTooOld(backendResponse, match.entry) - && (request.getEntity() == null || request.getEntity().isRepeatable())) { + if (revalidationResponseIsTooOld(backendResponse, match.entry)) { final ClassicHttpRequest unconditional = conditionalRequestBuilder.buildUnconditionalRequest(request); return callBackend(target, unconditional, scope, chain); } diff --git a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/ResponseCacheControl.java b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/ResponseCacheControl.java index 33469b533f..4e548bd4dc 100644 --- a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/ResponseCacheControl.java +++ b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/ResponseCacheControl.java @@ -27,7 +27,9 @@ package org.apache.hc.client5.http.impl.cache; +import java.util.Arrays; import java.util.Collections; +import java.util.HashSet; import java.util.Set; import org.apache.hc.core5.annotation.Contract; @@ -226,7 +228,6 @@ public boolean isMustRevalidate() { return mustRevalidate; } - /** * Returns whether the proxy-revalidate value is set in the Cache-Control header. * @@ -433,6 +434,12 @@ public Builder setNoCacheFields(final Set noCacheFields) { return this; } + public Builder setNoCacheFields(final String... noCacheFields) { + this.noCacheFields = new HashSet<>(); + this.noCacheFields.addAll(Arrays.asList(noCacheFields)); + return this; + } + public boolean isMustUnderstand() { return mustUnderstand; } diff --git a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/ResponseCachingPolicy.java b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/ResponseCachingPolicy.java index 47472f06d6..67abff2ff8 100644 --- a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/ResponseCachingPolicy.java +++ b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/ResponseCachingPolicy.java @@ -29,7 +29,6 @@ import java.time.Duration; import java.time.Instant; import java.util.Iterator; -import java.util.Set; import org.apache.hc.client5.http.cache.HttpCacheEntry; import org.apache.hc.client5.http.utils.DateUtils; @@ -397,43 +396,6 @@ boolean isStaleIfErrorEnabled(final ResponseCacheControl cacheControl, final Htt return false; } - /** - * Determines if the given {@link HttpCacheEntry} requires revalidation based on the presence of the {@code no-cache} directive - * in the Cache-Control header. - *

- * The method returns true in the following cases: - * - If the {@code no-cache} directive is present without any field names. - * - If the {@code no-cache} directive is present with field names, and at least one of these field names is present - * in the headers of the {@link HttpCacheEntry}. - *

- * If the {@code no-cache} directive is not present in the Cache-Control header, the method returns {@code false}. - * - * @param entry the {@link HttpCacheEntry} containing the headers to check for the {@code no-cache} directive. - * @return true if revalidation is required based on the {@code no-cache} directive, {@code false} otherwise. - */ - boolean responseContainsNoCacheDirective(final ResponseCacheControl responseCacheControl, final HttpCacheEntry entry) { - final Set noCacheFields = responseCacheControl.getNoCacheFields(); - - // If no-cache directive is present and has no field names - if (responseCacheControl.isNoCache() && noCacheFields.isEmpty()) { - LOG.debug("No-cache directive present without field names. Revalidation required."); - return true; - } - - // If no-cache directive is present with field names - if (responseCacheControl.isNoCache()) { - for (final String field : noCacheFields) { - if (entry.getFirstHeader(field) != null) { - if (LOG.isDebugEnabled()) { - LOG.debug("No-cache directive field '{}' found in response headers. Revalidation required.", field); - } - return true; - } - } - } - return false; - } - /** * Understood status codes include: * - All 2xx (Successful) status codes (200-299) diff --git a/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestCacheKeyGenerator.java b/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestCacheKeyGenerator.java index 8a41bbb1bc..8a27c4937e 100644 --- a/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestCacheKeyGenerator.java +++ b/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestCacheKeyGenerator.java @@ -28,15 +28,20 @@ import java.net.URI; import java.net.URISyntaxException; +import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; +import java.util.Iterator; +import java.util.List; import org.apache.hc.client5.http.cache.HttpCacheEntry; import org.apache.hc.client5.http.classic.methods.HttpGet; +import org.apache.hc.core5.http.Header; import org.apache.hc.core5.http.HttpHeaders; import org.apache.hc.core5.http.HttpHost; import org.apache.hc.core5.http.HttpRequest; import org.apache.hc.core5.http.message.BasicHeader; +import org.apache.hc.core5.http.message.BasicHeaderIterator; import org.apache.hc.core5.http.message.BasicHttpRequest; import org.apache.hc.core5.http.support.BasicRequestBuilder; import org.junit.jupiter.api.Assertions; @@ -295,6 +300,41 @@ public void testGetURIWithQueryParameters() { "/full_episodes?foo=bar"))); } + private static Iterator

headers(final Header... headers) { + return new BasicHeaderIterator(headers, null); + } + + @Test + public void testNormalizeHeaderElements() { + final List tokens = new ArrayList<>(); + CacheKeyGenerator.normalizeElements(headers( + new BasicHeader("Accept-Encoding", "gzip,zip,deflate") + ), tokens::add); + Assertions.assertEquals(Arrays.asList("deflate", "gzip", "zip"), tokens); + + tokens.clear(); + CacheKeyGenerator.normalizeElements(headers( + new BasicHeader("Accept-Encoding", " gZip , Zip, , , deflate ") + ), tokens::add); + Assertions.assertEquals(Arrays.asList("deflate", "gzip", "zip"), tokens); + + tokens.clear(); + CacheKeyGenerator.normalizeElements(headers( + new BasicHeader("Accept-Encoding", "gZip,Zip,,"), + new BasicHeader("Accept-Encoding", " gZip,Zip,,,"), + new BasicHeader("Accept-Encoding", "gZip, ,,,deflate") + ), tokens::add); + Assertions.assertEquals(Arrays.asList("deflate", "gzip", "zip"), tokens); + + tokens.clear(); + CacheKeyGenerator.normalizeElements(headers( + new BasicHeader("Cookie", "name1 = value1 ; p1 = v1 ; P2 = \"v2\""), + new BasicHeader("Cookie", "name3;;;"), + new BasicHeader("Cookie", " name2 = \" value 2 \" ; ; ; ,,,") + ), tokens::add); + Assertions.assertEquals(Arrays.asList("name1=value1;p1=v1;p2=v2", "name2=\" value 2 \"", "name3"), tokens); + } + @Test public void testGetVariantKey() { final HttpRequest request = BasicRequestBuilder.get("/blah") diff --git a/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestCacheValidityPolicy.java b/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestCacheValidityPolicy.java index a8e9f407f1..11672d0992 100644 --- a/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestCacheValidityPolicy.java +++ b/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestCacheValidityPolicy.java @@ -29,7 +29,6 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; -import static org.junit.jupiter.api.Assertions.assertSame; import static org.junit.jupiter.api.Assertions.assertTrue; import java.time.Instant; @@ -244,26 +243,6 @@ public void testHeuristicFreshnessLifetimeIsNonNegative() { assertTrue(TimeValue.isNonNegative(impl.getHeuristicFreshnessLifetime(entry))); } - @Test - public void testResponseIsFreshIfFreshnessLifetimeExceedsCurrentAge() { - final HttpCacheEntry entry = HttpTestUtils.makeCacheEntry(); - final ResponseCacheControl responseCacheControl = ResponseCacheControl.builder().build(); - impl = new CacheValidityPolicy() { - @Override - public TimeValue getCurrentAge(final HttpCacheEntry e, final Instant d) { - assertSame(entry, e); - assertEquals(now, d); - return TimeValue.ofSeconds(6); - } - @Override - public TimeValue getFreshnessLifetime(final ResponseCacheControl cacheControl, final HttpCacheEntry e) { - assertSame(entry, e); - return TimeValue.ofSeconds(10); - } - }; - assertTrue(impl.isResponseFresh(responseCacheControl, entry, now)); - } - @Test public void testHeuristicFreshnessLifetimeCustomProperly() { final CacheConfig cacheConfig = CacheConfig.custom().setHeuristicDefaultLifetime(TimeValue.ofSeconds(10)) @@ -274,46 +253,6 @@ public void testHeuristicFreshnessLifetimeCustomProperly() { assertEquals(defaultFreshness, impl.getHeuristicFreshnessLifetime(entry)); } - @Test - public void testResponseIsNotFreshIfFreshnessLifetimeEqualsCurrentAge() { - final HttpCacheEntry entry = HttpTestUtils.makeCacheEntry(); - final ResponseCacheControl responseCacheControl = ResponseCacheControl.builder().build(); - impl = new CacheValidityPolicy() { - @Override - public TimeValue getCurrentAge(final HttpCacheEntry e, final Instant d) { - assertEquals(now, d); - assertSame(entry, e); - return TimeValue.ofSeconds(6); - } - @Override - public TimeValue getFreshnessLifetime(final ResponseCacheControl cacheControl, final HttpCacheEntry e) { - assertSame(entry, e); - return TimeValue.ofSeconds(6); - } - }; - assertFalse(impl.isResponseFresh(responseCacheControl, entry, now)); - } - - @Test - public void testResponseIsNotFreshIfCurrentAgeExceedsFreshnessLifetime() { - final HttpCacheEntry entry = HttpTestUtils.makeCacheEntry(); - final ResponseCacheControl responseCacheControl = ResponseCacheControl.builder().build(); - impl = new CacheValidityPolicy() { - @Override - public TimeValue getCurrentAge(final HttpCacheEntry e, final Instant d) { - assertEquals(now, d); - assertSame(entry, e); - return TimeValue.ofSeconds(10); - } - @Override - public TimeValue getFreshnessLifetime(final ResponseCacheControl cacheControl, final HttpCacheEntry e) { - assertSame(entry, e); - return TimeValue.ofSeconds(6); - } - }; - assertFalse(impl.isResponseFresh(responseCacheControl, entry, now)); - } - @Test public void testCacheEntryIsRevalidatableIfHeadersIncludeETag() { final HttpCacheEntry entry = HttpTestUtils.makeCacheEntry( diff --git a/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestCachedResponseSuitabilityChecker.java b/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestCachedResponseSuitabilityChecker.java index bd23f14b5a..051a9d597e 100644 --- a/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestCachedResponseSuitabilityChecker.java +++ b/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestCachedResponseSuitabilityChecker.java @@ -31,11 +31,12 @@ import org.apache.hc.client5.http.cache.HttpCacheEntry; import org.apache.hc.client5.http.utils.DateUtils; import org.apache.hc.core5.http.Header; +import org.apache.hc.core5.http.HttpHost; import org.apache.hc.core5.http.HttpRequest; -import org.apache.hc.core5.http.HttpStatus; import org.apache.hc.core5.http.Method; import org.apache.hc.core5.http.message.BasicHeader; import org.apache.hc.core5.http.message.BasicHttpRequest; +import org.apache.hc.core5.http.support.BasicRequestBuilder; import org.apache.hc.core5.util.TimeValue; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.BeforeEach; @@ -69,32 +70,204 @@ public void setUp() { impl = new CachedResponseSuitabilityChecker(CacheConfig.DEFAULT); } - private HttpCacheEntry getEntry(final Header[] headers) { - return HttpTestUtils.makeCacheEntry(elevenSecondsAgo, nineSecondsAgo, headers); + private HttpCacheEntry makeEntry(final Instant requestDate, + final Instant responseDate, + final Method method, + final String requestUri, + final Header[] requestHeaders, + final int status, + final Header[] responseHeaders) { + return HttpTestUtils.makeCacheEntry(requestDate, responseDate, method, requestUri, requestHeaders, + status, responseHeaders, HttpTestUtils.makeNullResource()); + } + + private HttpCacheEntry makeEntry(final Header... headers) { + return makeEntry(elevenSecondsAgo, nineSecondsAgo, Method.GET, "/foo", null, 200, headers); + } + + private HttpCacheEntry makeEntry(final Instant requestDate, + final Instant responseDate, + final Header... headers) { + return makeEntry(requestDate, responseDate, Method.GET, "/foo", null, 200, headers); + } + + private HttpCacheEntry makeEntry(final Method method, final String requestUri, final Header... headers) { + return makeEntry(elevenSecondsAgo, nineSecondsAgo, method, requestUri, null, 200, headers); + } + + private HttpCacheEntry makeEntry(final Method method, final String requestUri, final Header[] requestHeaders, + final int status, final Header[] responseHeaders) { + return makeEntry(elevenSecondsAgo, nineSecondsAgo, method, requestUri, requestHeaders, + status, responseHeaders); + } + + @Test + public void testRequestMethodMatch() { + request = new BasicHttpRequest("GET", "/foo"); + entry = makeEntry(Method.GET, "/foo", + new BasicHeader("Date", DateUtils.formatStandardDate(tenSecondsAgo))); + Assertions.assertTrue(impl.requestMethodMatch(request, entry)); + + request = new BasicHttpRequest("HEAD", "/foo"); + Assertions.assertTrue(impl.requestMethodMatch(request, entry)); + + request = new BasicHttpRequest("POST", "/foo"); + Assertions.assertFalse(impl.requestMethodMatch(request, entry)); + + request = new BasicHttpRequest("HEAD", "/foo"); + entry = makeEntry(Method.HEAD, "/foo", + new BasicHeader("Date", DateUtils.formatStandardDate(tenSecondsAgo))); + Assertions.assertTrue(impl.requestMethodMatch(request, entry)); + + request = new BasicHttpRequest("GET", "/foo"); + entry = makeEntry(Method.HEAD, "/foo", + new BasicHeader("Date", DateUtils.formatStandardDate(tenSecondsAgo))); + Assertions.assertFalse(impl.requestMethodMatch(request, entry)); + } + + @Test + public void testRequestUriMatch() { + request = new BasicHttpRequest("GET", "/foo"); + entry = makeEntry(Method.GET, "/foo", + new BasicHeader("Date", DateUtils.formatStandardDate(tenSecondsAgo))); + Assertions.assertTrue(impl.requestUriMatch(request, entry)); + + request = new BasicHttpRequest("GET", new HttpHost("some-host"), "/foo"); + entry = makeEntry(Method.GET, "http://some-host:80/foo", + new BasicHeader("Date", DateUtils.formatStandardDate(tenSecondsAgo))); + Assertions.assertTrue(impl.requestUriMatch(request, entry)); + + request = new BasicHttpRequest("GET", new HttpHost("Some-Host"), "/foo?bar"); + entry = makeEntry(Method.GET, "http://some-host:80/foo?bar", + new BasicHeader("Date", DateUtils.formatStandardDate(tenSecondsAgo))); + Assertions.assertTrue(impl.requestUriMatch(request, entry)); + + request = new BasicHttpRequest("GET", new HttpHost("some-other-host"), "/foo"); + entry = makeEntry(Method.GET, "http://some-host:80/foo", + new BasicHeader("Date", DateUtils.formatStandardDate(tenSecondsAgo))); + Assertions.assertFalse(impl.requestUriMatch(request, entry)); + + request = new BasicHttpRequest("GET", new HttpHost("some-host"), "/foo?huh"); + entry = makeEntry(Method.GET, "http://some-host:80/foo?bar", + new BasicHeader("Date", DateUtils.formatStandardDate(tenSecondsAgo))); + Assertions.assertFalse(impl.requestUriMatch(request, entry)); + } + + @Test + public void testRequestHeadersMatch() { + request = BasicRequestBuilder.get("/foo").build(); + entry = makeEntry( + Method.GET, "/foo", + new Header[]{}, + 200, + new Header[]{ + new BasicHeader("Date", DateUtils.formatStandardDate(tenSecondsAgo)) + }); + Assertions.assertTrue(impl.requestHeadersMatch(request, entry)); + + request = BasicRequestBuilder.get("/foo").build(); + entry = makeEntry( + Method.GET, "/foo", + new Header[]{}, + 200, + new Header[]{ + new BasicHeader("Date", DateUtils.formatStandardDate(tenSecondsAgo)), + new BasicHeader("Vary", "") + }); + Assertions.assertTrue(impl.requestHeadersMatch(request, entry)); + + request = BasicRequestBuilder.get("/foo") + .addHeader("Accept-Encoding", "blah") + .build(); + entry = makeEntry( + Method.GET, "/foo", + new Header[]{ + new BasicHeader("Accept-Encoding", "blah") + }, + 200, + new Header[]{ + new BasicHeader("Date", DateUtils.formatStandardDate(tenSecondsAgo)), + new BasicHeader("Vary", "Accept-Encoding") + }); + Assertions.assertTrue(impl.requestHeadersMatch(request, entry)); + + request = BasicRequestBuilder.get("/foo") + .addHeader("Accept-Encoding", "gzip, deflate, deflate , zip, ") + .build(); + entry = makeEntry( + Method.GET, "/foo", + new Header[]{ + new BasicHeader("Accept-Encoding", " gzip, zip, deflate") + }, + 200, + new Header[]{ + new BasicHeader("Date", DateUtils.formatStandardDate(tenSecondsAgo)), + new BasicHeader("Vary", "Accept-Encoding") + }); + Assertions.assertTrue(impl.requestHeadersMatch(request, entry)); + + request = BasicRequestBuilder.get("/foo") + .addHeader("Accept-Encoding", "gzip, deflate, zip") + .build(); + entry = makeEntry( + Method.GET, "/foo", + new Header[]{ + new BasicHeader("Accept-Encoding", " gzip, deflate") + }, + 200, + new Header[]{ + new BasicHeader("Date", DateUtils.formatStandardDate(tenSecondsAgo)), + new BasicHeader("Vary", "Accept-Encoding") + }); + Assertions.assertFalse(impl.requestHeadersMatch(request, entry)); + } + + @Test + public void testResponseNoCache() { + entry = makeEntry(new BasicHeader("Date", DateUtils.formatStandardDate(tenSecondsAgo))); + responseCacheControl = ResponseCacheControl.builder() + .setNoCache(false) + .build(); + + Assertions.assertFalse(impl.isResponseNoCache(responseCacheControl, entry)); + + responseCacheControl = ResponseCacheControl.builder() + .setNoCache(true) + .build(); + + Assertions.assertTrue(impl.isResponseNoCache(responseCacheControl, entry)); + + responseCacheControl = ResponseCacheControl.builder() + .setNoCache(true) + .setNoCacheFields("stuff", "more-stuff") + .build(); + + Assertions.assertFalse(impl.isResponseNoCache(responseCacheControl, entry)); + + entry = makeEntry( + new BasicHeader("Date", DateUtils.formatStandardDate(tenSecondsAgo)), + new BasicHeader("stuff", "booh")); + + Assertions.assertTrue(impl.isResponseNoCache(responseCacheControl, entry)); } @Test public void testSuitableIfCacheEntryIsFresh() { - final Header[] headers = { - new BasicHeader("Date", DateUtils.formatStandardDate(tenSecondsAgo)) - }; - entry = getEntry(headers); + entry = makeEntry(new BasicHeader("Date", DateUtils.formatStandardDate(tenSecondsAgo))); responseCacheControl = ResponseCacheControl.builder() .setMaxAge(3600) .build(); - Assertions.assertTrue(impl.canCachedResponseBeUsed(requestCacheControl, responseCacheControl, request, entry, now)); + Assertions.assertEquals(CacheSuitability.FRESH, impl.assessSuitability(requestCacheControl, responseCacheControl, request, entry, now)); } @Test public void testNotSuitableIfCacheEntryIsNotFresh() { - final Header[] headers = { - new BasicHeader("Date", DateUtils.formatStandardDate(tenSecondsAgo)) - }; - entry = getEntry(headers); + entry = makeEntry( + new BasicHeader("Date", DateUtils.formatStandardDate(tenSecondsAgo))); responseCacheControl = ResponseCacheControl.builder() .setMaxAge(5) .build(); - Assertions.assertFalse(impl.canCachedResponseBeUsed(requestCacheControl, responseCacheControl, request, entry, now)); + Assertions.assertEquals(CacheSuitability.STALE, impl.assessSuitability(requestCacheControl, responseCacheControl, request, entry, now)); } @Test @@ -102,14 +275,12 @@ public void testNotSuitableIfRequestHasNoCache() { requestCacheControl = RequestCacheControl.builder() .setNoCache(true) .build(); - final Header[] headers = { - new BasicHeader("Date", DateUtils.formatStandardDate(tenSecondsAgo)) - }; - entry = getEntry(headers); + entry = makeEntry( + new BasicHeader("Date", DateUtils.formatStandardDate(tenSecondsAgo))); responseCacheControl = ResponseCacheControl.builder() .setMaxAge(3600) .build(); - Assertions.assertFalse(impl.canCachedResponseBeUsed(requestCacheControl, responseCacheControl, request, entry, now)); + Assertions.assertEquals(CacheSuitability.REVALIDATION_REQUIRED, impl.assessSuitability(requestCacheControl, responseCacheControl, request, entry, now)); } @Test @@ -117,14 +288,12 @@ public void testNotSuitableIfAgeExceedsRequestMaxAge() { requestCacheControl = RequestCacheControl.builder() .setMaxAge(10) .build(); - final Header[] headers = { - new BasicHeader("Date", DateUtils.formatStandardDate(tenSecondsAgo)) - }; responseCacheControl = ResponseCacheControl.builder() .setMaxAge(3600) .build(); - entry = getEntry(headers); - Assertions.assertFalse(impl.canCachedResponseBeUsed(requestCacheControl, responseCacheControl, request, entry, now)); + entry = makeEntry( + new BasicHeader("Date", DateUtils.formatStandardDate(tenSecondsAgo))); + Assertions.assertEquals(CacheSuitability.REVALIDATION_REQUIRED, impl.assessSuitability(requestCacheControl, responseCacheControl, request, entry, now)); } @Test @@ -132,14 +301,12 @@ public void testSuitableIfFreshAndAgeIsUnderRequestMaxAge() { requestCacheControl = RequestCacheControl.builder() .setMaxAge(15) .build(); - final Header[] headers = { - new BasicHeader("Date", DateUtils.formatStandardDate(tenSecondsAgo)) - }; - entry = getEntry(headers); + entry = makeEntry( + new BasicHeader("Date", DateUtils.formatStandardDate(tenSecondsAgo))); responseCacheControl = ResponseCacheControl.builder() .setMaxAge(3600) .build(); - Assertions.assertTrue(impl.canCachedResponseBeUsed(requestCacheControl, responseCacheControl, request, entry, now)); + Assertions.assertEquals(CacheSuitability.FRESH, impl.assessSuitability(requestCacheControl, responseCacheControl, request, entry, now)); } @Test @@ -147,14 +314,12 @@ public void testSuitableIfFreshAndFreshnessLifetimeGreaterThanRequestMinFresh() requestCacheControl = RequestCacheControl.builder() .setMinFresh(10) .build(); - final Header[] headers = { - new BasicHeader("Date", DateUtils.formatStandardDate(tenSecondsAgo)) - }; - entry = getEntry(headers); + entry = makeEntry( + new BasicHeader("Date", DateUtils.formatStandardDate(tenSecondsAgo))); responseCacheControl = ResponseCacheControl.builder() .setMaxAge(3600) .build(); - Assertions.assertTrue(impl.canCachedResponseBeUsed(requestCacheControl, responseCacheControl, request, entry, now)); + Assertions.assertEquals(CacheSuitability.FRESH, impl.assessSuitability(requestCacheControl, responseCacheControl, request, entry, now)); } @Test @@ -162,14 +327,12 @@ public void testNotSuitableIfFreshnessLifetimeLessThanRequestMinFresh() { requestCacheControl = RequestCacheControl.builder() .setMinFresh(10) .build(); - final Header[] headers = { - new BasicHeader("Date", DateUtils.formatStandardDate(tenSecondsAgo)) - }; - entry = getEntry(headers); + entry = makeEntry( + new BasicHeader("Date", DateUtils.formatStandardDate(tenSecondsAgo))); responseCacheControl = ResponseCacheControl.builder() .setMaxAge(15) .build(); - Assertions.assertFalse(impl.canCachedResponseBeUsed(requestCacheControl, responseCacheControl, request, entry, now)); + Assertions.assertEquals(CacheSuitability.REVALIDATION_REQUIRED, impl.assessSuitability(requestCacheControl, responseCacheControl, request, entry, now)); } @Test @@ -180,11 +343,11 @@ public void testSuitableEvenIfStaleButPermittedByRequestMaxStale() { final Header[] headers = { new BasicHeader("Date", DateUtils.formatStandardDate(tenSecondsAgo)) }; - entry = getEntry(headers); + entry = makeEntry(headers); responseCacheControl = ResponseCacheControl.builder() .setMaxAge(5) .build(); - Assertions.assertTrue(impl.canCachedResponseBeUsed(requestCacheControl, responseCacheControl, request, entry, now)); + Assertions.assertEquals(CacheSuitability.FRESH_ENOUGH, impl.assessSuitability(requestCacheControl, responseCacheControl, request, entry, now)); } @Test @@ -192,14 +355,12 @@ public void testNotSuitableIfStaleButTooStaleForRequestMaxStale() { requestCacheControl = RequestCacheControl.builder() .setMaxStale(2) .build(); - final Header[] headers = { - new BasicHeader("Date", DateUtils.formatStandardDate(tenSecondsAgo)) - }; - entry = getEntry(headers); + entry = makeEntry( + new BasicHeader("Date", DateUtils.formatStandardDate(tenSecondsAgo))); responseCacheControl = ResponseCacheControl.builder() .setMaxAge(5) .build(); - Assertions.assertFalse(impl.canCachedResponseBeUsed(requestCacheControl, responseCacheControl, request, entry, now)); + Assertions.assertEquals(CacheSuitability.REVALIDATION_REQUIRED, impl.assessSuitability(requestCacheControl, responseCacheControl, request, entry, now)); } @Test @@ -207,28 +368,22 @@ public void testSuitableIfCacheEntryIsHeuristicallyFreshEnough() { final Instant oneSecondAgo = now.minusSeconds(1); final Instant twentyOneSecondsAgo = now.minusSeconds(21); - final Header[] headers = { + entry = makeEntry(oneSecondAgo, oneSecondAgo, new BasicHeader("Date", DateUtils.formatStandardDate(oneSecondAgo)), - new BasicHeader("Last-Modified", DateUtils.formatStandardDate(twentyOneSecondsAgo)) - }; - - entry = HttpTestUtils.makeCacheEntry(oneSecondAgo, oneSecondAgo, headers); + new BasicHeader("Last-Modified", DateUtils.formatStandardDate(twentyOneSecondsAgo))); final CacheConfig config = CacheConfig.custom() .setHeuristicCachingEnabled(true) .setHeuristicCoefficient(0.1f).build(); impl = new CachedResponseSuitabilityChecker(config); - Assertions.assertTrue(impl.canCachedResponseBeUsed(requestCacheControl, responseCacheControl, request, entry, now)); + Assertions.assertEquals(CacheSuitability.FRESH, impl.assessSuitability(requestCacheControl, responseCacheControl, request, entry, now)); } @Test public void testSuitableIfCacheEntryIsHeuristicallyFreshEnoughByDefault() { - final Header[] headers = { - new BasicHeader("Date", DateUtils.formatStandardDate(tenSecondsAgo)) - }; - - entry = getEntry(headers); + entry = makeEntry( + new BasicHeader("Date", DateUtils.formatStandardDate(tenSecondsAgo))); final CacheConfig config = CacheConfig.custom() .setHeuristicCachingEnabled(true) @@ -236,71 +391,43 @@ public void testSuitableIfCacheEntryIsHeuristicallyFreshEnoughByDefault() { .build(); impl = new CachedResponseSuitabilityChecker(config); - Assertions.assertTrue(impl.canCachedResponseBeUsed(requestCacheControl, responseCacheControl, request, entry, now)); + Assertions.assertEquals(CacheSuitability.FRESH, impl.assessSuitability(requestCacheControl, responseCacheControl, request, entry, now)); } @Test public void testSuitableIfRequestMethodisHEAD() { final HttpRequest headRequest = new BasicHttpRequest("HEAD", "/foo"); - final Header[] headers = { - new BasicHeader("Date", DateUtils.formatStandardDate(tenSecondsAgo)) - }; - entry = getEntry(headers); + entry = makeEntry( + new BasicHeader("Date", DateUtils.formatStandardDate(tenSecondsAgo))); responseCacheControl = ResponseCacheControl.builder() .setMaxAge(3600) .build(); - Assertions.assertTrue(impl.canCachedResponseBeUsed(requestCacheControl, responseCacheControl, headRequest, entry, now)); - } - - @Test - public void testNotSuitableIfRequestMethodIsGETAndEntryResourceIsNull() { - final Header[] headers = { - new BasicHeader("Date", DateUtils.formatStandardDate(tenSecondsAgo)) - }; - entry = HttpTestUtils.makeCacheEntry(elevenSecondsAgo, nineSecondsAgo, - Method.HEAD, "/", null, - HttpStatus.SC_OK, headers, - HttpTestUtils.makeNullResource()); - responseCacheControl = ResponseCacheControl.builder() - .setMaxAge(3600) - .build(); - - Assertions.assertFalse(impl.canCachedResponseBeUsed(requestCacheControl, responseCacheControl, request, entry, now)); + Assertions.assertEquals(CacheSuitability.FRESH, impl.assessSuitability(requestCacheControl, responseCacheControl, headRequest, entry, now)); } @Test public void testSuitableForGETIfEntryDoesNotSpecifyARequestMethodButContainsEntity() { impl = new CachedResponseSuitabilityChecker(CacheConfig.custom().build()); - final Header[] headers = { - new BasicHeader("Date", DateUtils.formatStandardDate(tenSecondsAgo)) - }; - entry = HttpTestUtils.makeCacheEntry(elevenSecondsAgo, nineSecondsAgo, - Method.GET, "/", null, - HttpStatus.SC_OK, headers, - HttpTestUtils.makeRandomResource(128)); + entry = makeEntry(Method.GET, "/foo", + new BasicHeader("Date", DateUtils.formatStandardDate(tenSecondsAgo))); responseCacheControl = ResponseCacheControl.builder() .setMaxAge(3600) .build(); - Assertions.assertTrue(impl.canCachedResponseBeUsed(requestCacheControl, responseCacheControl, request, entry, now)); + Assertions.assertEquals(CacheSuitability.FRESH, impl.assessSuitability(requestCacheControl, responseCacheControl, request, entry, now)); } @Test public void testSuitableForGETIfHeadResponseCachingEnabledAndEntryDoesNotSpecifyARequestMethodButContains204Response() { impl = new CachedResponseSuitabilityChecker(CacheConfig.custom().build()); - final Header[] headers = { - new BasicHeader("Date", DateUtils.formatStandardDate(tenSecondsAgo)), - }; - entry = HttpTestUtils.makeCacheEntry(elevenSecondsAgo, nineSecondsAgo, - Method.GET, "/", null, - HttpStatus.SC_OK, headers, - HttpTestUtils.makeNullResource()); + entry = makeEntry(Method.GET, "/foo", + new BasicHeader("Date", DateUtils.formatStandardDate(tenSecondsAgo))); responseCacheControl = ResponseCacheControl.builder() .setMaxAge(3600) .build(); - Assertions.assertTrue(impl.canCachedResponseBeUsed(requestCacheControl, responseCacheControl, request, entry, now)); + Assertions.assertEquals(CacheSuitability.FRESH, impl.assessSuitability(requestCacheControl, responseCacheControl, request, entry, now)); } @Test @@ -308,33 +435,26 @@ public void testSuitableForHEADIfHeadResponseCachingEnabledAndEntryDoesNotSpecif final HttpRequest headRequest = new BasicHttpRequest("HEAD", "/foo"); impl = new CachedResponseSuitabilityChecker(CacheConfig.custom().build()); final Header[] headers = { - new BasicHeader("Date", DateUtils.formatStandardDate(tenSecondsAgo)) + }; - entry = HttpTestUtils.makeCacheEntry(elevenSecondsAgo, nineSecondsAgo, - Method.GET, "/", null, - HttpStatus.SC_OK, headers, - HttpTestUtils.makeNullResource()); + entry = makeEntry(Method.GET, "/foo", + new BasicHeader("Date", DateUtils.formatStandardDate(tenSecondsAgo))); responseCacheControl = ResponseCacheControl.builder() .setMaxAge(3600) .build(); - Assertions.assertTrue(impl.canCachedResponseBeUsed(requestCacheControl, responseCacheControl, headRequest, entry, now)); + Assertions.assertEquals(CacheSuitability.FRESH, impl.assessSuitability(requestCacheControl, responseCacheControl, headRequest, entry, now)); } @Test public void testNotSuitableIfGetRequestWithHeadCacheEntry() { // Prepare a cache entry with HEAD method - final Header[] headers = { - new BasicHeader("Date", DateUtils.formatStandardDate(tenSecondsAgo)), - }; - entry = HttpTestUtils.makeCacheEntry(elevenSecondsAgo, nineSecondsAgo, - Method.HEAD, "/", null, - HttpStatus.SC_OK, headers, - HttpTestUtils.makeNullResource()); + entry = makeEntry(Method.HEAD, "/foo", + new BasicHeader("Date", DateUtils.formatStandardDate(tenSecondsAgo))); responseCacheControl = ResponseCacheControl.builder() .setMaxAge(3600) .build(); // Validate that the cache entry is not suitable for the GET request - Assertions.assertFalse(impl.canCachedResponseBeUsed(requestCacheControl, responseCacheControl, request, entry, now)); + Assertions.assertEquals(CacheSuitability.MISMATCH, impl.assessSuitability(requestCacheControl, responseCacheControl, request, entry, now)); } } diff --git a/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestProtocolRequirements.java b/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestProtocolRequirements.java index 404a99b723..53a420fec4 100644 --- a/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestProtocolRequirements.java +++ b/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestProtocolRequirements.java @@ -53,6 +53,7 @@ import org.apache.hc.core5.http.HttpHeaders; import org.apache.hc.core5.http.HttpHost; import org.apache.hc.core5.http.HttpStatus; +import org.apache.hc.core5.http.Method; 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; @@ -102,7 +103,7 @@ public void setUp() throws Exception { body = HttpTestUtils.makeBody(ENTITY_LENGTH); - request = new BasicClassicHttpRequest("GET", "/foo"); + request = new BasicClassicHttpRequest("GET", "/"); context = HttpClientContext.create(); @@ -537,37 +538,6 @@ public void testCacheEntryIsUpdatedWithNewFieldValuesIn304Response() throws Exce Assertions.assertEquals("junk", result.getFirstHeader("X-Extra").getValue()); } - @Test - public void testMustNotUseMultipartByteRangeContentTypeOnCacheGenerated416Responses() throws Exception { - - originResponse.setEntity(HttpTestUtils.makeBody(ENTITY_LENGTH)); - originResponse.setHeader("Content-Length", "128"); - originResponse.setHeader("Cache-Control", "max-age=3600"); - - final ClassicHttpRequest rangeReq = new BasicClassicHttpRequest("GET", "/"); - rangeReq.setHeader("Range", "bytes=1000-1200"); - - final ClassicHttpResponse orig416 = new BasicClassicHttpResponse(416, - "Requested Range Not Satisfiable"); - - // cache may 416 me right away if it understands byte ranges, - // ok to delegate to origin though - Mockito.when(mockExecChain.proceed(Mockito.any(), Mockito.any())).thenReturn(originResponse); - Mockito.when(mockExecChain.proceed(RequestEquivalent.eq(rangeReq), Mockito.any())).thenReturn(orig416); - - execute(request); - final ClassicHttpResponse result = execute(rangeReq); - - // might have gotten a 416 from the origin or the cache - Assertions.assertEquals(416, result.getCode()); - final Iterator it = MessageSupport.iterate(result, HttpHeaders.CONTENT_TYPE); - while (it.hasNext()) { - final HeaderElement elt = it.next(); - Assertions.assertFalse("multipart/byteranges".equalsIgnoreCase(elt.getName())); - } - Mockito.verify(mockExecChain, Mockito.times(2)).proceed(Mockito.any(), Mockito.any()); - } - @Test public void testMustReturnACacheEntryIfItCanRevalidateIt() throws Exception { @@ -576,17 +546,12 @@ public void testMustReturnACacheEntryIfItCanRevalidateIt() throws Exception { final Instant nineSecondsAgo = now.minusSeconds(9); final Instant eightSecondsAgo = now.minusSeconds(8); - final Header[] hdrs = new Header[] { - new BasicHeader("Date", DateUtils.formatStandardDate(nineSecondsAgo)), - new BasicHeader("Cache-Control", "max-age=0"), - new BasicHeader("ETag", "\"etag\""), - new BasicHeader("Content-Length", "128") - }; - - final byte[] bytes = new byte[128]; - new Random().nextBytes(bytes); - - final HttpCacheEntry entry = HttpTestUtils.makeCacheEntry(tenSecondsAgo, eightSecondsAgo, hdrs, bytes); + final HttpCacheEntry entry = HttpTestUtils.makeCacheEntry(tenSecondsAgo, eightSecondsAgo, + Method.GET, "/thing", null, + 200, new Header[] { + new BasicHeader("Date", DateUtils.formatStandardDate(nineSecondsAgo)), + new BasicHeader("ETag", "\"etag\"") + }, HttpTestUtils.makeNullResource()); impl = new CachingExec(mockCache, null, config); @@ -602,6 +567,12 @@ public void testMustReturnACacheEntryIfItCanRevalidateIt() throws Exception { Mockito.when(mockCache.match(Mockito.eq(host), RequestEquivalent.eq(request))).thenReturn( new CacheMatch(new CacheHit("key", entry), null)); Mockito.when(mockExecChain.proceed(RequestEquivalent.eq(validate), Mockito.any())).thenReturn(notModified); + final HttpCacheEntry updated = HttpTestUtils.makeCacheEntry(tenSecondsAgo, eightSecondsAgo, + Method.GET, "/thing", null, + 200, new Header[] { + new BasicHeader("Date", DateUtils.formatStandardDate(now)), + new BasicHeader("ETag", "\"etag\"") + }, HttpTestUtils.makeNullResource()); Mockito.when(mockCache.update( Mockito.any(), Mockito.any(), @@ -609,7 +580,7 @@ public void testMustReturnACacheEntryIfItCanRevalidateIt() throws Exception { Mockito.any(), Mockito.any(), Mockito.any())) - .thenReturn(new CacheHit("key", HttpTestUtils.makeCacheEntry())); + .thenReturn(new CacheHit("key", updated)); execute(request); @@ -672,7 +643,7 @@ public void testAgeHeaderPopulatedFromCacheEntryCurrentAge() throws Exception { final HttpCacheEntry entry = HttpTestUtils.makeCacheEntry(tenSecondsAgo, eightSecondsAgo, hdrs, bytes); impl = new CachingExec(mockCache, null, config); - request = new BasicClassicHttpRequest("GET", "/thing"); + request = new BasicClassicHttpRequest("GET", "/"); Mockito.when(mockCache.match(Mockito.eq(host), RequestEquivalent.eq(request))).thenReturn( new CacheMatch(new CacheHit("key", entry), null)); From 772426c1ccc95ed6d2866809476671311bf60a7b Mon Sep 17 00:00:00 2001 From: Oleg Kalnichevski Date: Thu, 9 Nov 2023 20:43:54 +0100 Subject: [PATCH 5/8] HTTPCLIENT-2277: Revised cache validation logic for conformance with the specification requirements per RFC 9111 section 4 --- .../http/impl/cache/AsyncCachingExec.java | 193 ++++++++++++------ .../http/impl/cache/CacheSuitability.java | 2 + .../http/impl/cache/CacheValidityPolicy.java | 37 ---- .../CachedResponseSuitabilityChecker.java | 37 +++- .../client5/http/impl/cache/CachingExec.java | 124 ++++++----- .../http/impl/cache/CachingExecBase.java | 70 +------ .../impl/cache/ResponseCachingPolicy.java | 38 +--- .../impl/cache/TestCacheValidityPolicy.java | 124 ----------- .../TestCachedResponseSuitabilityChecker.java | 101 +++++++++ .../http/impl/cache/TestCachingExecChain.java | 22 +- .../cache/TestProtocolAllowedBehavior.java | 26 --- .../impl/cache/TestResponseCachingPolicy.java | 74 +++---- 12 files changed, 401 insertions(+), 447 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 fab7bff63b..2440c3d809 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 @@ -37,6 +37,7 @@ import java.util.concurrent.ScheduledExecutorService; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicReference; +import java.util.function.Consumer; import org.apache.hc.client5.http.HttpRoute; import org.apache.hc.client5.http.async.AsyncExecCallback; @@ -168,12 +169,12 @@ private void triggerResponse( static class AsyncExecCallbackWrapper implements AsyncExecCallback { - private final AsyncExecCallback asyncExecCallback; private final Runnable command; + private final Consumer exceptionConsumer; - AsyncExecCallbackWrapper(final AsyncExecCallback asyncExecCallback, final Runnable command) { - this.asyncExecCallback = asyncExecCallback; + AsyncExecCallbackWrapper(final Runnable command, final Consumer exceptionConsumer) { this.command = command; + this.exceptionConsumer = exceptionConsumer; } @Override @@ -194,7 +195,9 @@ public void completed() { @Override public void failed(final Exception cause) { - asyncExecCallback.failed(cause); + if (exceptionConsumer != null) { + exceptionConsumer.accept(cause); + } } } @@ -603,7 +606,7 @@ private void handleCacheHit( if (cacheSuitability == CacheSuitability.FRESH || cacheSuitability == CacheSuitability.FRESH_ENOUGH) { LOG.debug("Cache hit"); try { - final SimpleHttpResponse cacheResponse = generateCachedResponse(hit.entry, request, context); + final SimpleHttpResponse cacheResponse = generateCachedResponse(request, context, hit.entry); triggerResponse(cacheResponse, scope, asyncExecCallback); } catch (final ResourceIOException ex) { recordCacheFailure(target, request); @@ -631,17 +634,47 @@ private void handleCacheHit( LOG.debug("Request is not repeatable; calling backend"); callBackend(target, request, entityProducer, scope, chain, asyncExecCallback); } else if (hit.entry.getStatus() == HttpStatus.SC_NOT_MODIFIED && !suitabilityChecker.isConditional(request)) { - LOG.debug("Cache entry with NOT_MODIFIED does not match the non-conditional request; calling backend"); + LOG.debug("Non-modified cache entry does not match the non-conditional request; calling backend"); callBackend(target, request, entityProducer, scope, chain, asyncExecCallback); - } else if (cacheSuitability == CacheSuitability.REVALIDATION_REQUIRED || cacheSuitability == CacheSuitability.STALE) { - LOG.debug("Revalidating cache entry"); - final boolean staleIfErrorEnabled = responseCachingPolicy.isStaleIfErrorEnabled(responseCacheControl, hit.entry); - if (cacheRevalidator != null - && !staleResponseNotAllowed(requestCacheControl, responseCacheControl, hit.entry, now) - && (validityPolicy.mayReturnStaleWhileRevalidating(responseCacheControl, hit.entry, now) || staleIfErrorEnabled)) { + } else if (cacheSuitability == CacheSuitability.REVALIDATION_REQUIRED) { + LOG.debug("Revalidation required; revalidating cache entry"); + revalidateCacheEntry(responseCacheControl, hit, target, request, entityProducer, scope, chain, new AsyncExecCallback() { + + private final AtomicBoolean committed = new AtomicBoolean(); + + @Override + public AsyncDataConsumer handleResponse(final HttpResponse response, + final EntityDetails entityDetails) throws HttpException, IOException { + committed.set(true); + return asyncExecCallback.handleResponse(response, entityDetails); + } + + @Override + public void handleInformationResponse(final HttpResponse response) throws HttpException, IOException { + asyncExecCallback.handleInformationResponse(response); + } + + @Override + public void completed() { + asyncExecCallback.completed(); + } + + @Override + public void failed(final Exception cause) { + if (!committed.get() && cause instanceof IOException) { + final SimpleHttpResponse cacheResponse = generateGatewayTimeout(scope.clientContext); + LOG.debug(cause.getMessage(), cause); + triggerResponse(cacheResponse, scope, asyncExecCallback); + } else { + asyncExecCallback.failed(cause); + } + } + + }); + } else if (cacheSuitability == CacheSuitability.STALE_WHILE_REVALIDATED) { + if (cacheRevalidator != null) { LOG.debug("Serving stale with asynchronous revalidation"); try { - final SimpleHttpResponse cacheResponse = generateCachedResponse(hit.entry, request, context); final String exchangeId = ExecSupport.getNextExchangeId(); context.setExchangeId(exchangeId); final AsyncExecChain.Scope fork = new AsyncExecChain.Scope( @@ -656,30 +689,19 @@ private void handleCacheHit( cacheRevalidator.revalidateCacheEntry( hit.getEntryKey(), asyncExecCallback, - asyncExecCallback1 -> revalidateCacheEntry(requestCacheControl, responseCacheControl, - hit, target, request, entityProducer, fork, chain, asyncExecCallback1)); + c -> revalidateCacheEntry(responseCacheControl, hit, target, request, entityProducer, fork, chain, c)); + final SimpleHttpResponse cacheResponse = unvalidatedCacheHit(request, context, hit.entry); triggerResponse(cacheResponse, scope, asyncExecCallback); - } catch (final ResourceIOException ex) { - if (staleIfErrorEnabled) { - if (LOG.isDebugEnabled()) { - LOG.debug("Serving stale response due to IOException and stale-if-error enabled"); - } - try { - final SimpleHttpResponse cacheResponse = generateCachedResponse(hit.entry, request, context); - 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); - } + } catch (final IOException ex) { + asyncExecCallback.failed(ex); } } else { - revalidateCacheEntry(requestCacheControl, responseCacheControl, hit, target, request, entityProducer, scope, chain, asyncExecCallback); + LOG.debug("Revalidating stale cache entry (asynchronous revalidation disabled)"); + revalidateCacheEntryWithFallback(requestCacheControl, responseCacheControl, hit, target, request, entityProducer, scope, chain, asyncExecCallback); } + } else if (cacheSuitability == CacheSuitability.STALE) { + LOG.debug("Revalidating stale cache entry"); + revalidateCacheEntryWithFallback(requestCacheControl, responseCacheControl, hit, target, request, entityProducer, scope, chain, asyncExecCallback); } else { LOG.debug("Cache entry not usable; calling backend"); callBackend(target, request, entityProducer, scope, chain, asyncExecCallback); @@ -688,7 +710,6 @@ private void handleCacheHit( } void revalidateCacheEntry( - final RequestCacheControl requestCacheControl, final ResponseCacheControl responseCacheControl, final CacheHit hit, final HttpHost target, @@ -720,17 +741,11 @@ void triggerUpdatedCacheEntryResponse(final HttpResponse backendResponse, final @Override public void completed(final CacheHit updated) { - if (suitabilityChecker.isConditional(request) - && suitabilityChecker.allConditionalsMatch(request, updated.entry, Instant.now())) { - final SimpleHttpResponse cacheResponse = responseGenerator.generateNotModifiedResponse(updated.entry); + try { + final SimpleHttpResponse cacheResponse = generateCachedResponse(request, scope.clientContext, updated.entry); triggerResponse(cacheResponse, scope, asyncExecCallback); - } else { - try { - final SimpleHttpResponse cacheResponse = responseGenerator.generateResponse(request, updated.entry); - triggerResponse(cacheResponse, scope, asyncExecCallback); - } catch (final ResourceIOException ex) { - asyncExecCallback.failed(ex); - } + } catch (final ResourceIOException ex) { + asyncExecCallback.failed(ex); } } @@ -762,12 +777,7 @@ AsyncExecCallback evaluateResponse(final HttpResponse backendResponse, final Ins recordCacheUpdate(scope.clientContext); } if (statusCode == HttpStatus.SC_NOT_MODIFIED) { - return new AsyncExecCallbackWrapper(asyncExecCallback, () -> triggerUpdatedCacheEntryResponse(backendResponse, responseDate)); - } - if (staleIfErrorAppliesTo(statusCode) - && !staleResponseNotAllowed(requestCacheControl, responseCacheControl, hit.entry, getCurrentDate()) - && validityPolicy.mayReturnStaleIfError(requestCacheControl, responseCacheControl, hit.entry, responseDate)) { - return new AsyncExecCallbackWrapper(asyncExecCallback, this::triggerResponseStaleCacheEntry); + return new AsyncExecCallbackWrapper(() -> triggerUpdatedCacheEntryResponse(backendResponse, responseDate), asyncExecCallback::failed); } return new BackendResponseHandler(target, conditionalRequest, requestDate, responseDate, scope, asyncExecCallback); } @@ -780,12 +790,12 @@ public AsyncDataConsumer handleResponse( final Instant responseDate1 = getCurrentDate(); final AsyncExecCallback callback1; - if (revalidationResponseIsTooOld(backendResponse1, hit.entry)) { + if (HttpCacheEntry.isNewer(hit.entry, backendResponse1)) { final HttpRequest unconditional = conditionalRequestBuilder.buildUnconditionalRequest( BasicRequestBuilder.copy(scope.originalRequest).build()); - callback1 = new AsyncExecCallbackWrapper(asyncExecCallback, () -> chainProceed(unconditional, entityProducer, scope, chain, new AsyncExecCallback() { + callback1 = new AsyncExecCallbackWrapper(() -> chainProceed(unconditional, entityProducer, scope, chain, new AsyncExecCallback() { @Override public AsyncDataConsumer handleResponse( @@ -827,7 +837,7 @@ public void failed(final Exception cause) { } } - })); + }), asyncExecCallback::failed); } else { callback1 = evaluateResponse(backendResponse1, responseDate1); } @@ -869,6 +879,71 @@ public void failed(final Exception cause) { } + void revalidateCacheEntryWithFallback( + final RequestCacheControl requestCacheControl, + final ResponseCacheControl responseCacheControl, + final CacheHit hit, + final HttpHost target, + final HttpRequest request, + final AsyncEntityProducer entityProducer, + final AsyncExecChain.Scope scope, + final AsyncExecChain chain, + final AsyncExecCallback asyncExecCallback) { + revalidateCacheEntry(responseCacheControl, hit, target, request, entityProducer, scope, chain, new AsyncExecCallback() { + + private final AtomicBoolean committed = new AtomicBoolean(); + + @Override + public AsyncDataConsumer handleResponse(final HttpResponse response, final EntityDetails entityDetails) throws HttpException, IOException { + final int status = response.getCode(); + if (staleIfErrorAppliesTo(status) && + suitabilityChecker.isSuitableIfError(requestCacheControl, responseCacheControl, hit.entry, getCurrentDate())) { + return null; + } else { + committed.set(true); + return asyncExecCallback.handleResponse(response, entityDetails); + } + } + + @Override + public void handleInformationResponse(final HttpResponse response) throws HttpException, IOException { + asyncExecCallback.handleInformationResponse(response); + } + + @Override + public void completed() { + if (committed.get()) { + asyncExecCallback.completed(); + } else { + try { + final SimpleHttpResponse cacheResponse = unvalidatedCacheHit(request, scope.clientContext, hit.entry); + triggerResponse(cacheResponse, scope, asyncExecCallback); + } catch (final IOException ex) { + asyncExecCallback.failed(ex); + } + } + } + + @Override + public void failed(final Exception cause) { + if (!committed.get() && + cause instanceof IOException && + suitabilityChecker.isSuitableIfError(requestCacheControl, responseCacheControl, hit.entry, getCurrentDate())) { + try { + final SimpleHttpResponse cacheResponse = unvalidatedCacheHit(request, scope.clientContext, hit.entry); + triggerResponse(cacheResponse, scope, asyncExecCallback); + } catch (final IOException ex) { + asyncExecCallback.failed(cause); + } + } else { + LOG.debug(cause.getMessage(), cause); + final SimpleHttpResponse cacheResponse = generateGatewayTimeout(scope.clientContext); + triggerResponse(cacheResponse, scope, asyncExecCallback); + } + } + + }); + } private void handleCacheMiss( final RequestCacheControl requestCacheControl, final CacheHit partialMatch, @@ -954,7 +1029,7 @@ void updateVariantCacheEntry(final HttpResponse backendResponse, final Instant r @Override public void completed(final CacheHit hit) { - if (shouldSendNotModifiedResponse(request, hit.entry)) { + if (shouldSendNotModifiedResponse(request, hit.entry, Instant.now())) { final SimpleHttpResponse cacheResponse = responseGenerator.generateNotModifiedResponse(hit.entry); triggerResponse(cacheResponse, scope, asyncExecCallback); } else { @@ -1055,21 +1130,21 @@ public void cancelled() { final Header resultEtagHeader = backendResponse.getFirstHeader(HttpHeaders.ETAG); if (resultEtagHeader == null) { LOG.warn("304 response did not contain ETag"); - callback = new AsyncExecCallbackWrapper(asyncExecCallback, () -> callBackend(target, request, entityProducer, scope, chain, asyncExecCallback)); + callback = new AsyncExecCallbackWrapper(() -> callBackend(target, request, entityProducer, scope, chain, asyncExecCallback), asyncExecCallback::failed); } else { final String resultEtag = resultEtagHeader.getValue(); final CacheHit match = variantMap.get(resultEtag); if (match == null) { LOG.debug("304 response did not contain ETag matching one sent in If-None-Match"); - callback = new AsyncExecCallbackWrapper(asyncExecCallback, () -> callBackend(target, request, entityProducer, scope, chain, asyncExecCallback)); + callback = new AsyncExecCallbackWrapper(() -> callBackend(target, request, entityProducer, scope, chain, asyncExecCallback), asyncExecCallback::failed); } else { - if (revalidationResponseIsTooOld(backendResponse, match.entry)) { + if (HttpCacheEntry.isNewer(match.entry, backendResponse)) { final HttpRequest unconditional = conditionalRequestBuilder.buildUnconditionalRequest( BasicRequestBuilder.copy(request).build()); scope.clientContext.setAttribute(HttpCoreContext.HTTP_REQUEST, unconditional); - callback = new AsyncExecCallbackWrapper(asyncExecCallback, () -> callBackend(target, request, entityProducer, scope, chain, asyncExecCallback)); + callback = new AsyncExecCallbackWrapper(() -> callBackend(target, request, entityProducer, scope, chain, asyncExecCallback), asyncExecCallback::failed); } else { - callback = new AsyncExecCallbackWrapper(asyncExecCallback, () -> updateVariantCacheEntry(backendResponse, responseDate, match)); + callback = new AsyncExecCallbackWrapper(() -> updateVariantCacheEntry(backendResponse, responseDate, match), asyncExecCallback::failed); } } } diff --git a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CacheSuitability.java b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CacheSuitability.java index 2431489414..7a7e290554 100644 --- a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CacheSuitability.java +++ b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CacheSuitability.java @@ -37,6 +37,8 @@ enum CacheSuitability { FRESH, // the cache entry is fresh and can be used to satisfy the request FRESH_ENOUGH, // the cache entry is deemed fresh enough and can be used to satisfy the request STALE, // the cache entry is stale and may be unsuitable to satisfy the request + STALE_WHILE_REVALIDATED, // the cache entry is stale but may be unsuitable to satisfy the request + // while being re-validated at the same time REVALIDATION_REQUIRED // the cache entry is stale and must not be used to satisfy the request // without revalidation diff --git a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CacheValidityPolicy.java b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CacheValidityPolicy.java index ca0dbb4635..2b2a2d52b1 100644 --- a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CacheValidityPolicy.java +++ b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CacheValidityPolicy.java @@ -146,34 +146,6 @@ TimeValue getHeuristicFreshnessLifetime(final HttpCacheEntry entry) { return heuristicDefaultLifetime; } - public boolean isRevalidatable(final HttpCacheEntry entry) { - return entry.containsHeader(HttpHeaders.ETAG) || entry.containsHeader(HttpHeaders.LAST_MODIFIED); - } - - public boolean mayReturnStaleWhileRevalidating(final ResponseCacheControl responseCacheControl, - final HttpCacheEntry entry, final Instant now) { - if (responseCacheControl.getStaleWhileRevalidate() >= 0) { - final TimeValue staleness = getStaleness(responseCacheControl, entry, now); - if (staleness.compareTo(TimeValue.ofSeconds(responseCacheControl.getStaleWhileRevalidate())) <= 0) { - return true; - } - } - return false; - } - - public boolean mayReturnStaleIfError(final RequestCacheControl requestCacheControl, - final ResponseCacheControl responseCacheControl, final HttpCacheEntry entry, - final Instant now) { - final TimeValue staleness = getStaleness(responseCacheControl, entry, now); - return mayReturnStaleIfError(requestCacheControl, staleness) || - mayReturnStaleIfError(responseCacheControl, staleness); - } - - private boolean mayReturnStaleIfError(final CacheControl responseCacheControl, final TimeValue staleness) { - return responseCacheControl.getStaleIfError() >= 0 && - staleness.compareTo(TimeValue.ofSeconds(responseCacheControl.getStaleIfError())) <= 0; - } - TimeValue getApparentAge(final HttpCacheEntry entry) { final Instant dateValue = entry.getInstant(); if (dateValue == null) { @@ -238,13 +210,4 @@ TimeValue getResidentTime(final HttpCacheEntry entry, final Instant now) { return TimeValue.ofSeconds(diff.getSeconds()); } - TimeValue getStaleness(final ResponseCacheControl responseCacheControl, final HttpCacheEntry entry, final Instant now) { - final TimeValue age = getCurrentAge(entry, now); - final TimeValue freshness = getFreshnessLifetime(responseCacheControl, entry); - if (age.compareTo(freshness) <= 0) { - return TimeValue.ZERO_MILLISECONDS; - } - return TimeValue.ofSeconds(age.toSeconds() - freshness.toSeconds()); - } - } diff --git a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachedResponseSuitabilityChecker.java b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachedResponseSuitabilityChecker.java index 69a053540b..a81a59716d 100644 --- a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachedResponseSuitabilityChecker.java +++ b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachedResponseSuitabilityChecker.java @@ -58,14 +58,16 @@ class CachedResponseSuitabilityChecker { private static final Logger LOG = LoggerFactory.getLogger(CachedResponseSuitabilityChecker.class); - private final boolean sharedCache; private final CacheValidityPolicy validityStrategy; + private final boolean sharedCache; + private final boolean staleifError; CachedResponseSuitabilityChecker(final CacheValidityPolicy validityStrategy, final CacheConfig config) { super(); this.validityStrategy = validityStrategy; this.sharedCache = config.isSharedCache(); + this.staleifError = config.isStaleIfErrorEnabled(); } CachedResponseSuitabilityChecker(final CacheConfig config) { @@ -173,6 +175,13 @@ public CacheSuitability assessSuitability(final RequestCacheControl requestCache LOG.debug("The cache entry is fresh"); return CacheSuitability.FRESH; } else { + if (responseCacheControl.getStaleWhileRevalidate() > 0) { + final long stale = currentAge.compareTo(freshnessLifetime) > 0 ? currentAge.toSeconds() - freshnessLifetime.toSeconds() : 0; + if (stale < responseCacheControl.getStaleWhileRevalidate()) { + LOG.debug("The cache entry is stale but suitable while being revalidated"); + return CacheSuitability.STALE_WHILE_REVALIDATED; + } + } LOG.debug("The cache entry is stale"); return CacheSuitability.STALE; } @@ -349,4 +358,30 @@ boolean lastModifiedValidatorMatches(final HttpRequest request, final HttpCacheE return true; } + public boolean isSuitableIfError(final RequestCacheControl requestCacheControl, + final ResponseCacheControl responseCacheControl, + final HttpCacheEntry entry, + final Instant now) { + // Explicit cache control + if (requestCacheControl.getStaleIfError() > 0 || responseCacheControl.getStaleIfError() > 0) { + final TimeValue currentAge = validityStrategy.getCurrentAge(entry, now); + final TimeValue freshnessLifetime = validityStrategy.getFreshnessLifetime(responseCacheControl, entry); + if (requestCacheControl.getMinFresh() > 0 && requestCacheControl.getMinFresh() < freshnessLifetime.toSeconds()) { + return false; + } + final long stale = currentAge.compareTo(freshnessLifetime) > 0 ? currentAge.toSeconds() - freshnessLifetime.toSeconds() : 0; + if (requestCacheControl.getStaleIfError() > 0 && stale < requestCacheControl.getStaleIfError()) { + return true; + } + if (responseCacheControl.getStaleIfError() > 0 && stale < responseCacheControl.getStaleIfError()) { + return true; + } + } + // Global override + if (staleifError && requestCacheControl.getStaleIfError() == -1 && responseCacheControl.getStaleIfError() == -1) { + return true; + } + return false; + } + } \ No newline at end of file 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 6348033a57..5e1f4bb143 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 @@ -59,6 +59,7 @@ import org.apache.hc.core5.http.HttpStatus; import org.apache.hc.core5.http.HttpVersion; import org.apache.hc.core5.http.io.entity.ByteArrayEntity; +import org.apache.hc.core5.http.io.entity.EntityUtils; import org.apache.hc.core5.http.io.entity.StringEntity; import org.apache.hc.core5.http.io.support.ClassicRequestBuilder; import org.apache.hc.core5.http.message.BasicClassicHttpResponse; @@ -251,7 +252,7 @@ private ClassicHttpResponse handleCacheHit( if (cacheSuitability == CacheSuitability.FRESH || cacheSuitability == CacheSuitability.FRESH_ENOUGH) { LOG.debug("Cache hit"); try { - return convert(generateCachedResponse(hit.entry, request, context), scope); + return convert(generateCachedResponse(request, context, hit.entry), scope); } catch (final ResourceIOException ex) { recordCacheFailure(target, request); if (!mayCallBackend(requestCacheControl)) { @@ -271,40 +272,39 @@ private ClassicHttpResponse handleCacheHit( LOG.debug("Request is not repeatable; calling backend"); return callBackend(target, request, scope, chain); } else if (hit.entry.getStatus() == HttpStatus.SC_NOT_MODIFIED && !suitabilityChecker.isConditional(request)) { - LOG.debug("Cache entry with NOT_MODIFIED does not match the non-conditional request; calling backend"); + LOG.debug("Non-modified cache entry does not match the non-conditional request; calling backend"); return callBackend(target, request, scope, chain); - } else if (cacheSuitability == CacheSuitability.REVALIDATION_REQUIRED || cacheSuitability == CacheSuitability.STALE) { - LOG.debug("Revalidating cache entry"); - final boolean staleIfErrorEnabled = responseCachingPolicy.isStaleIfErrorEnabled(responseCacheControl, hit.entry); + } else if (cacheSuitability == CacheSuitability.REVALIDATION_REQUIRED) { + LOG.debug("Revalidation required; revalidating cache entry"); try { - if (cacheRevalidator != null - && !staleResponseNotAllowed(requestCacheControl, responseCacheControl, hit.entry, now) - && (validityPolicy.mayReturnStaleWhileRevalidating(responseCacheControl, hit.entry, now) || staleIfErrorEnabled)) { - LOG.debug("Serving stale with asynchronous revalidation"); - final String exchangeId = ExecSupport.getNextExchangeId(); - context.setExchangeId(exchangeId); - final ExecChain.Scope fork = new ExecChain.Scope( - exchangeId, - scope.route, - scope.originalRequest, - scope.execRuntime.fork(null), - HttpClientContext.create()); - final SimpleHttpResponse response = generateCachedResponse(hit.entry, request, context); - cacheRevalidator.revalidateCacheEntry( - hit.getEntryKey(), - () -> revalidateCacheEntry(requestCacheControl, responseCacheControl, hit, target, request, fork, chain)); - return convert(response, scope); - } - return revalidateCacheEntry(requestCacheControl, responseCacheControl, hit, target, request, scope, chain); - } 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(hit.entry, request, context), scope); - } - return convert(handleRevalidationFailure(requestCacheControl, responseCacheControl, hit.entry, request, context, now), scope); + return revalidateCacheEntry(responseCacheControl, hit, target, request, scope, chain); + } catch (final IOException ex) { + LOG.debug(ex.getMessage(), ex); + return convert(generateGatewayTimeout(scope.clientContext), scope); } + } else if (cacheSuitability == CacheSuitability.STALE_WHILE_REVALIDATED) { + if (cacheRevalidator != null) { + LOG.debug("Serving stale with asynchronous revalidation"); + final String exchangeId = ExecSupport.getNextExchangeId(); + context.setExchangeId(exchangeId); + final ExecChain.Scope fork = new ExecChain.Scope( + exchangeId, + scope.route, + scope.originalRequest, + scope.execRuntime.fork(null), + HttpClientContext.create()); + cacheRevalidator.revalidateCacheEntry( + hit.getEntryKey(), + () -> revalidateCacheEntry(responseCacheControl, hit, target, request, fork, chain)); + final SimpleHttpResponse response = unvalidatedCacheHit(request, context, hit.entry); + return convert(response, scope); + } else { + LOG.debug("Revalidating stale cache entry (asynchronous revalidation disabled)"); + return revalidateCacheEntryWithFallback(requestCacheControl, responseCacheControl, hit, target, request, scope, chain); + } + } else if (cacheSuitability == CacheSuitability.STALE) { + LOG.debug("Revalidating stale cache entry"); + return revalidateCacheEntryWithFallback(requestCacheControl, responseCacheControl, hit, target, request, scope, chain); } else { LOG.debug("Cache entry not usable; calling backend"); return callBackend(target, request, scope, chain); @@ -313,7 +313,6 @@ private ClassicHttpResponse handleCacheHit( } ClassicHttpResponse revalidateCacheEntry( - final RequestCacheControl requestCacheControl, final ResponseCacheControl responseCacheControl, final CacheHit hit, final HttpHost target, @@ -328,7 +327,7 @@ ClassicHttpResponse revalidateCacheEntry( try { Instant responseDate = getCurrentDate(); - if (revalidationResponseIsTooOld(backendResponse, hit.entry)) { + if (HttpCacheEntry.isNewer(hit.entry, backendResponse)) { backendResponse.close(); final ClassicHttpRequest unconditional = conditionalRequestBuilder.buildUnconditionalRequest( scope.originalRequest); @@ -341,25 +340,9 @@ ClassicHttpResponse revalidateCacheEntry( if (statusCode == HttpStatus.SC_NOT_MODIFIED || statusCode == HttpStatus.SC_OK) { recordCacheUpdate(scope.clientContext); } - if (statusCode == HttpStatus.SC_NOT_MODIFIED) { final CacheHit updated = responseCache.update(hit, target, request, backendResponse, requestDate, responseDate); - if (suitabilityChecker.isConditional(request) - && suitabilityChecker.allConditionalsMatch(request, updated.entry, Instant.now())) { - return convert(responseGenerator.generateNotModifiedResponse(updated.entry), scope); - } - return convert(responseGenerator.generateResponse(request, updated.entry), scope); - } - - if (staleIfErrorAppliesTo(statusCode) - && !staleResponseNotAllowed(requestCacheControl, responseCacheControl, hit.entry, getCurrentDate()) - && validityPolicy.mayReturnStaleIfError(requestCacheControl, responseCacheControl, hit.entry, responseDate)) { - try { - final SimpleHttpResponse cachedResponse = responseGenerator.generateResponse(request, hit.entry); - return convert(cachedResponse, scope); - } finally { - backendResponse.close(); - } + return convert(generateCachedResponse(request, scope.clientContext, updated.entry), scope); } return handleBackendResponse(target, conditionalRequest, scope, requestDate, responseDate, backendResponse); } catch (final IOException | RuntimeException ex) { @@ -368,6 +351,41 @@ ClassicHttpResponse revalidateCacheEntry( } } + ClassicHttpResponse revalidateCacheEntryWithFallback( + final RequestCacheControl requestCacheControl, + final ResponseCacheControl responseCacheControl, + final CacheHit hit, + final HttpHost target, + final ClassicHttpRequest request, + final ExecChain.Scope scope, + final ExecChain chain) throws HttpException, IOException { + final HttpClientContext context = scope.clientContext; + final ClassicHttpResponse response; + try { + response = revalidateCacheEntry(responseCacheControl, hit, target, request, scope, chain); + } catch (final IOException ex) { + if (suitabilityChecker.isSuitableIfError(requestCacheControl, responseCacheControl, hit.entry, getCurrentDate())) { + if (LOG.isDebugEnabled()) { + LOG.debug("Serving stale response due to IOException and stale-if-error enabled"); + } + return convert(unvalidatedCacheHit(request, context, hit.entry), scope); + } else { + LOG.debug(ex.getMessage(), ex); + return convert(generateGatewayTimeout(context), scope); + } + } + final int status = response.getCode(); + if (staleIfErrorAppliesTo(status) && + suitabilityChecker.isSuitableIfError(requestCacheControl, responseCacheControl, hit.entry, getCurrentDate())) { + if (LOG.isDebugEnabled()) { + LOG.debug("Serving stale response due to {} status and stale-if-error enabled", status); + } + EntityUtils.consume(response.getEntity()); + return convert(unvalidatedCacheHit(request, context, hit.entry), scope); + } + return response; + } + ClassicHttpResponse handleBackendResponse( final HttpHost target, final ClassicHttpRequest request, @@ -518,7 +536,7 @@ ClassicHttpResponse negotiateResponseFromVariants( return callBackend(target, request, scope, chain); } - if (revalidationResponseIsTooOld(backendResponse, match.entry)) { + if (HttpCacheEntry.isNewer(match.entry, backendResponse)) { final ClassicHttpRequest unconditional = conditionalRequestBuilder.buildUnconditionalRequest(request); return callBackend(target, unconditional, scope, chain); } @@ -526,7 +544,7 @@ ClassicHttpResponse negotiateResponseFromVariants( recordCacheUpdate(scope.clientContext); final CacheHit hit = responseCache.storeFromNegotiated(match, target, request, backendResponse, requestDate, responseDate); - if (shouldSendNotModifiedResponse(request, hit.entry)) { + if (shouldSendNotModifiedResponse(request, hit.entry, Instant.now())) { return convert(responseGenerator.generateNotModifiedResponse(hit.entry), scope); } else { return convert(responseGenerator.generateResponse(request, hit.entry), scope); diff --git a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachingExecBase.java b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachingExecBase.java index 81bb1e4c31..a26aec2e1b 100644 --- a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachingExecBase.java +++ b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachingExecBase.java @@ -44,7 +44,6 @@ import org.apache.hc.core5.http.HttpStatus; import org.apache.hc.core5.http.Method; import org.apache.hc.core5.http.protocol.HttpContext; -import org.apache.hc.core5.util.TimeValue; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -88,8 +87,7 @@ public class CachingExecBase { this.responseCachingPolicy = new ResponseCachingPolicy( this.cacheConfig.isSharedCache(), this.cacheConfig.isNeverCacheHTTP10ResponsesWithQuery(), - this.cacheConfig.isNeverCacheHTTP11ResponsesWithQuery(), - this.cacheConfig.isStaleIfErrorEnabled()); + this.cacheConfig.isNeverCacheHTTP11ResponsesWithQuery()); } /** @@ -146,31 +144,14 @@ void recordCacheUpdate(final HttpContext context) { } SimpleHttpResponse generateCachedResponse( - final HttpCacheEntry entry, - final HttpRequest request, - final HttpContext context) throws ResourceIOException { - final SimpleHttpResponse cachedResponse; - if (request.containsHeader(HttpHeaders.IF_NONE_MATCH) - || request.containsHeader(HttpHeaders.IF_MODIFIED_SINCE)) { - cachedResponse = responseGenerator.generateNotModifiedResponse(entry); - } else { - cachedResponse = responseGenerator.generateResponse(request, entry); - } - setResponseStatus(context, CacheResponseStatus.CACHE_HIT); - return cachedResponse; - } - - SimpleHttpResponse handleRevalidationFailure( - final RequestCacheControl requestCacheControl, - final ResponseCacheControl responseCacheControl, - final HttpCacheEntry entry, final HttpRequest request, final HttpContext context, - final Instant now) throws IOException { - if (staleResponseNotAllowed(requestCacheControl, responseCacheControl, entry, now)) { - return generateGatewayTimeout(context); + final HttpCacheEntry entry) throws ResourceIOException { + setResponseStatus(context, CacheResponseStatus.CACHE_HIT); + if (shouldSendNotModifiedResponse(request, entry, Instant.now())) { + return responseGenerator.generateNotModifiedResponse(entry); } else { - return unvalidatedCacheHit(request, context, entry); + return responseGenerator.generateResponse(request, entry); } } @@ -189,15 +170,6 @@ SimpleHttpResponse unvalidatedCacheHit( return cachedResponse; } - boolean staleResponseNotAllowed(final RequestCacheControl requestCacheControl, - final ResponseCacheControl responseCacheControl, - final HttpCacheEntry entry, - final Instant now) { - return responseCacheControl.isMustRevalidate() - || (cacheConfig.isSharedCache() && responseCacheControl.isProxyRevalidate()) - || explicitFreshnessRequest(requestCacheControl, responseCacheControl, entry, now); - } - boolean mayCallBackend(final RequestCacheControl requestCacheControl) { if (requestCacheControl.isOnlyIfCached()) { LOG.debug("Request marked only-if-cached"); @@ -206,22 +178,6 @@ boolean mayCallBackend(final RequestCacheControl requestCacheControl) { return true; } - boolean explicitFreshnessRequest(final RequestCacheControl requestCacheControl, - final ResponseCacheControl responseCacheControl, - final HttpCacheEntry entry, - final Instant now) { - if (requestCacheControl.getMaxStale() >= 0) { - final TimeValue age = validityPolicy.getCurrentAge(entry, now); - final TimeValue lifetime = validityPolicy.getFreshnessLifetime(responseCacheControl, entry); - if (age.toSeconds() - lifetime.toSeconds() > requestCacheControl.getMaxStale()) { - return true; - } - } else if (requestCacheControl.getMinFresh() >= 0 || requestCacheControl.getMaxAge() >= 0) { - return true; - } - return false; - } - void setResponseStatus(final HttpContext context, final CacheResponseStatus value) { if (context != null) { context.setAttribute(HttpCacheContext.CACHE_RESPONSE_STATUS, value); @@ -245,17 +201,9 @@ boolean clientRequestsOurOptions(final HttpRequest request) { return "0".equals(h != null ? h.getValue() : null); } - boolean revalidationResponseIsTooOld(final HttpResponse backendResponse, final HttpCacheEntry cacheEntry) { - // either backend response or cached entry did not have a valid - // Date header, so we can't tell if they are out of order - // according to the origin clock; thus we can skip the - // unconditional retry. - return HttpCacheEntry.isNewer(cacheEntry, backendResponse); - } - - boolean shouldSendNotModifiedResponse(final HttpRequest request, final HttpCacheEntry responseEntry) { - return (suitabilityChecker.isConditional(request) - && suitabilityChecker.allConditionalsMatch(request, responseEntry, Instant.now())); + boolean shouldSendNotModifiedResponse(final HttpRequest request, final HttpCacheEntry responseEntry, final Instant now) { + return suitabilityChecker.isConditional(request) + && suitabilityChecker.allConditionalsMatch(request, responseEntry, now); } boolean staleIfErrorAppliesTo(final int statusCode) { diff --git a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/ResponseCachingPolicy.java b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/ResponseCachingPolicy.java index 67abff2ff8..31dd0f3dd0 100644 --- a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/ResponseCachingPolicy.java +++ b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/ResponseCachingPolicy.java @@ -30,7 +30,6 @@ import java.time.Instant; import java.util.Iterator; -import org.apache.hc.client5.http.cache.HttpCacheEntry; import org.apache.hc.client5.http.utils.DateUtils; import org.apache.hc.core5.http.HttpHeaders; import org.apache.hc.core5.http.HttpRequest; @@ -63,14 +62,6 @@ class ResponseCachingPolicy { private final boolean neverCache1_0ResponsesWithQueryString; private final boolean neverCache1_1ResponsesWithQueryString; - /** - * A flag indicating whether serving stale cache entries is allowed when an error occurs - * while fetching a fresh response from the origin server. - * If {@code true}, stale cache entries may be served in case of errors. - * If {@code false}, stale cache entries will not be served in case of errors. - */ - private final boolean staleIfErrorEnabled; - /** * Constructs a new ResponseCachingPolicy with the specified cache policy settings and stale-if-error support. * @@ -80,20 +71,15 @@ class ResponseCachingPolicy { * {@code false} to cache if explicit cache headers are found. * @param neverCache1_1ResponsesWithQueryString {@code true} to never cache HTTP 1.1 responses with a query string, * {@code false} to cache if explicit cache headers are found. - * @param staleIfErrorEnabled {@code true} to enable the stale-if-error cache directive, which - * allows clients to receive a stale cache entry when a request - * results in an error, {@code false} to disable this feature. * @since 5.3 */ public ResponseCachingPolicy( final boolean sharedCache, final boolean neverCache1_0ResponsesWithQueryString, - final boolean neverCache1_1ResponsesWithQueryString, - final boolean staleIfErrorEnabled) { + final boolean neverCache1_1ResponsesWithQueryString) { this.sharedCache = sharedCache; this.neverCache1_0ResponsesWithQueryString = neverCache1_0ResponsesWithQueryString; this.neverCache1_1ResponsesWithQueryString = neverCache1_1ResponsesWithQueryString; - this.staleIfErrorEnabled = staleIfErrorEnabled; } /** @@ -374,28 +360,6 @@ private Duration calculateFreshnessLifetime(final ResponseCacheControl cacheCont return DEFAULT_FRESHNESS_DURATION; // 5 minutes } - /** - * Determines whether a stale response should be served in case of an error status code in the cached response. - * This method first checks if the {@code stale-if-error} extension is enabled in the cache configuration. If it is, it - * then checks if the cached response has an error status code (500-504). If it does, it checks if the response has a - * {@code stale-while-revalidate} directive in its Cache-Control header. If it does, this method returns {@code true}, - * indicating that a stale response can be served. If not, it returns {@code false}. - * - * @return {@code true} if a stale response can be served in case of an error status code, {@code false} otherwise - */ - boolean isStaleIfErrorEnabled(final ResponseCacheControl cacheControl, final HttpCacheEntry entry) { - // Check if the stale-while-revalidate extension is enabled - if (staleIfErrorEnabled) { - // Check if the cached response has an error status code - final int statusCode = entry.getStatus(); - if (statusCode >= HttpStatus.SC_INTERNAL_SERVER_ERROR && statusCode <= HttpStatus.SC_GATEWAY_TIMEOUT) { - // Check if the cached response has a stale-while-revalidate directive - return cacheControl.getStaleWhileRevalidate() > 0; - } - } - return false; - } - /** * Understood status codes include: * - All 2xx (Successful) status codes (200-299) diff --git a/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestCacheValidityPolicy.java b/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestCacheValidityPolicy.java index 11672d0992..02cf52fcd7 100644 --- a/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestCacheValidityPolicy.java +++ b/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestCacheValidityPolicy.java @@ -28,7 +28,6 @@ import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertTrue; import java.time.Instant; @@ -253,30 +252,6 @@ public void testHeuristicFreshnessLifetimeCustomProperly() { assertEquals(defaultFreshness, impl.getHeuristicFreshnessLifetime(entry)); } - @Test - public void testCacheEntryIsRevalidatableIfHeadersIncludeETag() { - final HttpCacheEntry entry = HttpTestUtils.makeCacheEntry( - new BasicHeader("Expires", DateUtils.formatStandardDate(Instant.now())), - new BasicHeader("ETag", "somevalue")); - assertTrue(impl.isRevalidatable(entry)); - } - - @Test - public void testCacheEntryIsRevalidatableIfHeadersIncludeLastModifiedDate() { - final HttpCacheEntry entry = HttpTestUtils.makeCacheEntry( - new BasicHeader("Expires", DateUtils.formatStandardDate(Instant.now())), - new BasicHeader("Last-Modified", DateUtils.formatStandardDate(Instant.now()))); - assertTrue(impl.isRevalidatable(entry)); - } - - @Test - public void testCacheEntryIsNotRevalidatableIfNoAppropriateHeaders() { - final HttpCacheEntry entry = HttpTestUtils.makeCacheEntry( - new BasicHeader("Expires", DateUtils.formatStandardDate(Instant.now())), - new BasicHeader("Cache-Control", "public")); - assertFalse(impl.isRevalidatable(entry)); - } - @Test public void testNegativeAgeHeaderValueReturnsZero() { final Header[] headers = new Header[] { new BasicHeader("Age", "-100") }; @@ -326,103 +301,4 @@ public void testMalformedAgeHeaderOverflow() { assertEquals(0, impl.getAgeValue(entry)); } - - @Test - public void testMayReturnStaleIfErrorInResponseIsTrueWithinStaleness(){ - final HttpCacheEntry entry = HttpTestUtils.makeCacheEntry(now, now, - new BasicHeader("Date", DateUtils.formatStandardDate(tenSecondsAgo))); - final RequestCacheControl requestCacheControl = RequestCacheControl.builder() - .build(); - final ResponseCacheControl responseCacheControl = ResponseCacheControl.builder() - .setMaxAge(5) - .setStaleIfError(15) - .build(); - assertTrue(impl.mayReturnStaleIfError(requestCacheControl, responseCacheControl, entry, now)); - } - - @Test - public void testMayReturnStaleIfErrorInRequestIsTrueWithinStaleness(){ - final HttpCacheEntry entry = HttpTestUtils.makeCacheEntry(now, now, - new BasicHeader("Date", DateUtils.formatStandardDate(tenSecondsAgo))); - final RequestCacheControl requestCacheControl = RequestCacheControl.builder() - .build(); - final ResponseCacheControl responseCacheControl = ResponseCacheControl.builder() - .setStaleIfError(15) - .build(); - assertTrue(impl.mayReturnStaleIfError(requestCacheControl, responseCacheControl, entry, now)); - } - - @Test - public void testMayNotReturnStaleIfErrorInResponseAndAfterResponseWindow(){ - final HttpCacheEntry entry = HttpTestUtils.makeCacheEntry(now, now, - new BasicHeader("Date", DateUtils.formatStandardDate(tenSecondsAgo))); - final RequestCacheControl requestCacheControl = RequestCacheControl.builder() - .build(); - final ResponseCacheControl responseCacheControl = ResponseCacheControl.builder() - .setMaxAge(5) - .setStaleIfError(1) - .build(); - assertFalse(impl.mayReturnStaleIfError(requestCacheControl, responseCacheControl, entry, now)); - } - - @Test - public void testMayNotReturnStaleIfErrorInResponseAndAfterRequestWindow(){ - final HttpCacheEntry entry = HttpTestUtils.makeCacheEntry(now, now, - new BasicHeader("Date", DateUtils.formatStandardDate(tenSecondsAgo)), - new BasicHeader("Cache-Control", "max-age=5")); - final RequestCacheControl requestCacheControl = RequestCacheControl.builder() - .setStaleIfError(1) - .build(); - final ResponseCacheControl responseCacheControl = ResponseCacheControl.builder() - .setMaxAge(5) - .build(); - assertFalse(impl.mayReturnStaleIfError(requestCacheControl, responseCacheControl, entry, now)); - } - - @Test - public void testMayReturnStaleWhileRevalidatingIsFalseWhenDirectiveIsAbsent() { - final HttpCacheEntry entry = HttpTestUtils.makeCacheEntry(); - final ResponseCacheControl responseCacheControl = ResponseCacheControl.builder() - .setCachePublic(true) - .build(); - - assertFalse(impl.mayReturnStaleWhileRevalidating(responseCacheControl, entry, now)); - } - - @Test - public void testMayReturnStaleWhileRevalidatingIsTrueWhenWithinStaleness() { - final HttpCacheEntry entry = HttpTestUtils.makeCacheEntry(now, now, - new BasicHeader("Date", DateUtils.formatStandardDate(tenSecondsAgo))); - final ResponseCacheControl responseCacheControl = ResponseCacheControl.builder() - .setMaxAge(5) - .setStaleWhileRevalidate(15) - .build(); - - assertTrue(impl.mayReturnStaleWhileRevalidating(responseCacheControl, entry, now)); - } - - @Test - public void testMayReturnStaleWhileRevalidatingIsFalseWhenPastStaleness() { - final Instant twentyFiveSecondsAgo = now.minusSeconds(25); - final HttpCacheEntry entry = HttpTestUtils.makeCacheEntry(now, now, - new BasicHeader("Date", DateUtils.formatStandardDate(twentyFiveSecondsAgo))); - final ResponseCacheControl responseCacheControl = ResponseCacheControl.builder() - .setMaxAge(5) - .setStaleWhileRevalidate(15) - .build(); - - assertFalse(impl.mayReturnStaleWhileRevalidating(responseCacheControl, entry, now)); - } - - @Test - public void testMayReturnStaleWhileRevalidatingIsFalseWhenDirectiveEmpty() { - final HttpCacheEntry entry = HttpTestUtils.makeCacheEntry(now, now, - new BasicHeader("Date", DateUtils.formatStandardDate(tenSecondsAgo))); - final ResponseCacheControl responseCacheControl = ResponseCacheControl.builder() - .setMaxAge(5) - .setStaleWhileRevalidate(0) - .build(); - - assertFalse(impl.mayReturnStaleWhileRevalidating(responseCacheControl, entry, now)); - } } diff --git a/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestCachedResponseSuitabilityChecker.java b/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestCachedResponseSuitabilityChecker.java index 051a9d597e..0b2ce981e9 100644 --- a/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestCachedResponseSuitabilityChecker.java +++ b/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestCachedResponseSuitabilityChecker.java @@ -457,4 +457,105 @@ public void testNotSuitableIfGetRequestWithHeadCacheEntry() { // Validate that the cache entry is not suitable for the GET request Assertions.assertEquals(CacheSuitability.MISMATCH, impl.assessSuitability(requestCacheControl, responseCacheControl, request, entry, now)); } + + @Test + public void testSuitableIfErrorRequestCacheControl() { + // Prepare a cache entry with HEAD method + entry = makeEntry(Method.GET, "/foo", + new BasicHeader("Date", DateUtils.formatStandardDate(tenSecondsAgo))); + responseCacheControl = ResponseCacheControl.builder() + .setMaxAge(5) + .build(); + + // the entry has been stale for 6 seconds + + requestCacheControl = RequestCacheControl.builder() + .setStaleIfError(10) + .build(); + Assertions.assertTrue(impl.isSuitableIfError(requestCacheControl, responseCacheControl, entry, now)); + + requestCacheControl = RequestCacheControl.builder() + .setStaleIfError(5) + .build(); + Assertions.assertFalse(impl.isSuitableIfError(requestCacheControl, responseCacheControl, entry, now)); + + requestCacheControl = RequestCacheControl.builder() + .setStaleIfError(10) + .setMinFresh(4) // should take precedence over stale-if-error + .build(); + Assertions.assertFalse(impl.isSuitableIfError(requestCacheControl, responseCacheControl, entry, now)); + + requestCacheControl = RequestCacheControl.builder() + .setStaleIfError(-1) // not set or not valid + .build(); + Assertions.assertFalse(impl.isSuitableIfError(requestCacheControl, responseCacheControl, entry, now)); + } + + @Test + public void testSuitableIfErrorResponseCacheControl() { + // Prepare a cache entry with HEAD method + entry = makeEntry(Method.GET, "/foo", + new BasicHeader("Date", DateUtils.formatStandardDate(tenSecondsAgo))); + responseCacheControl = ResponseCacheControl.builder() + .setMaxAge(5) + .setStaleIfError(10) + .build(); + + // the entry has been stale for 6 seconds + + Assertions.assertTrue(impl.isSuitableIfError(requestCacheControl, responseCacheControl, entry, now)); + + responseCacheControl = ResponseCacheControl.builder() + .setMaxAge(5) + .setStaleIfError(5) + .build(); + Assertions.assertFalse(impl.isSuitableIfError(requestCacheControl, responseCacheControl, entry, now)); + + responseCacheControl = ResponseCacheControl.builder() + .setMaxAge(5) + .setStaleIfError(-1) // not set or not valid + .build(); + Assertions.assertFalse(impl.isSuitableIfError(requestCacheControl, responseCacheControl, entry, now)); + } + + @Test + public void testSuitableIfErrorRequestCacheControlTakesPrecedenceOverResponseCacheControl() { + // Prepare a cache entry with HEAD method + entry = makeEntry(Method.GET, "/foo", + new BasicHeader("Date", DateUtils.formatStandardDate(tenSecondsAgo))); + responseCacheControl = ResponseCacheControl.builder() + .setMaxAge(5) + .setStaleIfError(5) + .build(); + + // the entry has been stale for 6 seconds + + Assertions.assertFalse(impl.isSuitableIfError(requestCacheControl, responseCacheControl, entry, now)); + + requestCacheControl = RequestCacheControl.builder() + .setStaleIfError(10) + .build(); + Assertions.assertTrue(impl.isSuitableIfError(requestCacheControl, responseCacheControl, entry, now)); + } + + @Test + public void testSuitableIfErrorConfigDefault() { + // Prepare a cache entry with HEAD method + entry = makeEntry(Method.GET, "/foo", + new BasicHeader("Date", DateUtils.formatStandardDate(tenSecondsAgo))); + responseCacheControl = ResponseCacheControl.builder() + .setMaxAge(5) + .build(); + impl = new CachedResponseSuitabilityChecker(CacheConfig.custom() + .setStaleIfErrorEnabled(true) + .build()); + Assertions.assertTrue(impl.isSuitableIfError(requestCacheControl, responseCacheControl, entry, now)); + + requestCacheControl = RequestCacheControl.builder() + .setStaleIfError(5) + .build(); + + Assertions.assertFalse(impl.isSuitableIfError(requestCacheControl, responseCacheControl, entry, now)); + } + } 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 1a31989d4d..c7b120f15f 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 @@ -589,7 +589,7 @@ public void testSetsModuleResponseContextIfValidationFailsButNotRequired() throw Mockito.when(mockExecChain.proceed(Mockito.any(), Mockito.any())).thenThrow(new IOException()); execute(req2); - Assertions.assertEquals(CacheResponseStatus.CACHE_HIT, context.getCacheResponseStatus()); + Assertions.assertEquals(CacheResponseStatus.CACHE_MODULE_RESPONSE, context.getCacheResponseStatus()); } @Test @@ -1220,33 +1220,31 @@ public void testReturnssetStaleIfErrorEnabled() throws Exception { // 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"); + final ClassicHttpResponse resp1 = new BasicClassicHttpResponse(HttpStatus.SC_OK, "OK"); resp1.setEntity(HttpTestUtils.makeBody(128)); resp1.setHeader("Content-Length", "128"); - resp1.setHeader("ETag", "\"etag\""); + resp1.setHeader("ETag", "\"abc\""); resp1.setHeader("Date", DateUtils.formatStandardDate(Instant.now().minus(Duration.ofHours(10)))); - resp1.setHeader("Cache-Control", "public, max-age=-1, stale-while-revalidate=1"); + resp1.setHeader("Cache-Control", "public, stale-while-revalidate=1"); + + final BasicClassicHttpRequest req2 = new BasicClassicHttpRequest("GET", "http://foo.example.com/"); req2.addHeader("If-None-Match", "\"abc\""); - final ClassicHttpResponse resp2 = HttpTestUtils.make200Response(); + final ClassicHttpResponse resp2 = HttpTestUtils.make500Response(); // Set up the mock response chain Mockito.when(mockExecChain.proceed(Mockito.any(), Mockito.any())).thenReturn(resp1); - // Execute the first request and assert the response final ClassicHttpResponse response1 = execute(req1); - Assertions.assertEquals(HttpStatus.SC_GATEWAY_TIMEOUT, response1.getCode()); - + Assertions.assertEquals(HttpStatus.SC_OK, response1.getCode()); // Execute the second request and assert the response Mockito.when(mockExecRuntime.fork(Mockito.any())).thenReturn(mockExecRuntime); Mockito.when(mockExecChain.proceed(Mockito.any(), Mockito.any())).thenReturn(resp2); final ClassicHttpResponse response2 = execute(req2); - Assertions.assertEquals(HttpStatus.SC_NOT_MODIFIED, response2.getCode()); + Assertions.assertEquals(HttpStatus.SC_OK, response2.getCode()); - // Assert that the cache revalidator was called - Mockito.verify(cacheRevalidator, Mockito.times(1)).revalidateCacheEntry(Mockito.any(), Mockito.any()); + Mockito.verify(cacheRevalidator, Mockito.never()).revalidateCacheEntry(Mockito.any(), Mockito.any()); } @Test diff --git a/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestProtocolAllowedBehavior.java b/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestProtocolAllowedBehavior.java index a89e2d8989..3bfffac31d 100644 --- a/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestProtocolAllowedBehavior.java +++ b/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestProtocolAllowedBehavior.java @@ -27,14 +27,11 @@ package org.apache.hc.client5.http.impl.cache; import java.io.IOException; -import java.net.SocketTimeoutException; -import java.time.Instant; import org.apache.hc.client5.http.HttpRoute; import org.apache.hc.client5.http.classic.ExecChain; import org.apache.hc.client5.http.classic.ExecRuntime; 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.HttpException; @@ -107,29 +104,6 @@ public ClassicHttpResponse execute(final ClassicHttpRequest request) throws IOEx mockExecChain); } - @Test - public void testNonSharedCacheReturnsStaleResponseWhenRevalidationFailsForProxyRevalidate() throws Exception { - final ClassicHttpRequest req1 = new BasicClassicHttpRequest("GET","/"); - final Instant now = Instant.now(); - final Instant tenSecondsAgo = now.minusSeconds(10); - originResponse.setHeader("Date", DateUtils.formatStandardDate(tenSecondsAgo)); - originResponse.setHeader("Cache-Control","max-age=5,proxy-revalidate"); - originResponse.setHeader("Etag","\"etag\""); - - final ClassicHttpRequest req2 = new BasicClassicHttpRequest("GET","/"); - - Mockito.when(mockExecChain.proceed(Mockito.any(), Mockito.any())).thenReturn(originResponse); - - execute(req1); - - Mockito.when(mockExecChain.proceed(Mockito.any(), Mockito.any())).thenThrow(new SocketTimeoutException()); - - final HttpResponse result = execute(req2); - Assertions.assertEquals(HttpStatus.SC_OK, result.getCode()); - - Mockito.verifyNoInteractions(mockCache); - } - @Test public void testNonSharedCacheMayCacheResponsesWithCacheControlPrivate() throws Exception { final ClassicHttpRequest req1 = new BasicClassicHttpRequest("GET","/"); diff --git a/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestResponseCachingPolicy.java b/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestResponseCachingPolicy.java index 9215e5c184..eb5b9a3dca 100644 --- a/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestResponseCachingPolicy.java +++ b/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestResponseCachingPolicy.java @@ -68,7 +68,7 @@ public void setUp() throws Exception { sixSecondsAgo = now.minusSeconds(6); tenSecondsFromNow = now.plusSeconds(10); - policy = new ResponseCachingPolicy(true, false, false, false); + policy = new ResponseCachingPolicy(true, false, false); request = new BasicHttpRequest("GET","/"); response = new BasicHttpResponse(HttpStatus.SC_OK, ""); response.setHeader("Date", DateUtils.formatStandardDate(Instant.now())); @@ -78,14 +78,14 @@ public void setUp() throws Exception { @Test public void testGetCacheable() { - policy = new ResponseCachingPolicy(true, false, false, true); + policy = new ResponseCachingPolicy(true, false, false); request = new BasicHttpRequest(Method.GET, "/"); Assertions.assertTrue(policy.isResponseCacheable(responseCacheControl, request, response)); } @Test public void testHeadCacheable() { - policy = new ResponseCachingPolicy(true, false, false, true); + policy = new ResponseCachingPolicy(true, false, false); request = new BasicHttpRequest(Method.HEAD, "/"); Assertions.assertTrue(policy.isResponseCacheable(responseCacheControl, request, response)); } @@ -108,7 +108,7 @@ public void testResponsesToRequestsWithAuthorizationHeadersAreNotCacheableByShar @Test public void testResponsesToRequestsWithAuthorizationHeadersAreCacheableByNonSharedCache() { - policy = new ResponseCachingPolicy(false, false, false, true); + policy = new ResponseCachingPolicy(false, false, false); request = new BasicHttpRequest("GET","/"); request.setHeader("Authorization", StandardAuthScheme.BASIC + " dXNlcjpwYXNzd2Q="); Assertions.assertTrue(policy.isResponseCacheable(responseCacheControl, request, response)); @@ -191,7 +191,7 @@ public void testPlain303ResponseCodeIsNotCacheableUnderDefaultBehavior() { @Test public void testPlain303ResponseCodeIsNotCacheableEvenIf303CachingEnabled() { - policy = new ResponseCachingPolicy(true, false, true, true); + policy = new ResponseCachingPolicy(true, false, true); response.setCode(HttpStatus.SC_SEE_OTHER); response.removeHeaders("Expires"); Assertions.assertFalse(policy.isResponseCacheable(responseCacheControl, request, response)); @@ -283,7 +283,7 @@ public void testNon206WithPrivateCacheControlIsNotCacheableBySharedCache() { @Test public void test200ResponseWithPrivateCacheControlIsCacheableByNonSharedCache() { - policy = new ResponseCachingPolicy(false, false, false, true); + policy = new ResponseCachingPolicy(false, false, false); response.setCode(HttpStatus.SC_OK); responseCacheControl = ResponseCacheControl.builder() .setCachePrivate(true) @@ -387,7 +387,7 @@ public void testVaryStarIsNotCacheable() { @Test public void testVaryStarIsNotCacheableUsingSharedPublicCache() { - policy = new ResponseCachingPolicy(true, false, false, true); + policy = new ResponseCachingPolicy(true, false, false); request.setHeader("Authorization", StandardAuthScheme.BASIC + " QWxhZGRpbjpvcGVuIHNlc2FtZQ=="); response.setHeader("Vary", "*"); @@ -405,7 +405,7 @@ public void testRequestWithVaryHeaderCacheable() { @Test public void testIsArbitraryMethodCacheableUsingSharedPublicCache() { - policy = new ResponseCachingPolicy(true, false, false, true); + policy = new ResponseCachingPolicy(true, false, false); request = new HttpOptions("http://foo.example.com/"); request.setHeader("Authorization", StandardAuthScheme.BASIC + " QWxhZGRpbjpvcGVuIHNlc2FtZQ=="); @@ -426,7 +426,7 @@ public void testResponsesWithMultipleAgeHeadersAreCacheable() { @Test public void testResponsesWithMultipleAgeHeadersAreNotCacheableUsingSharedPublicCache() { - policy = new ResponseCachingPolicy(true, false, false, true); + policy = new ResponseCachingPolicy(true, false, false); request.setHeader("Authorization", StandardAuthScheme.BASIC + " QWxhZGRpbjpvcGVuIHNlc2FtZQ=="); response.addHeader("Age", "3"); @@ -446,7 +446,7 @@ public void testResponsesWithMultipleDateHeadersAreNotCacheable() { @Test public void testResponsesWithMultipleDateHeadersAreNotCacheableUsingSharedPublicCache() { - policy = new ResponseCachingPolicy(true, false, false, true); + policy = new ResponseCachingPolicy(true, false, false); request.setHeader("Authorization", StandardAuthScheme.BASIC + " QWxhZGRpbjpvcGVuIHNlc2FtZQ=="); response.addHeader("Date", DateUtils.formatStandardDate(now)); @@ -465,7 +465,7 @@ public void testResponsesWithMalformedDateHeadersAreNotCacheable() { @Test public void testResponsesWithMalformedDateHeadersAreNotCacheableUsingSharedPublicCache() { - policy = new ResponseCachingPolicy(true, false, false, true); + policy = new ResponseCachingPolicy(true, false, false); request.setHeader("Authorization", StandardAuthScheme.BASIC + " QWxhZGRpbjpvcGVuIHNlc2FtZQ=="); response.addHeader("Date", "garbage"); @@ -484,7 +484,7 @@ public void testResponsesWithMultipleExpiresHeadersAreNotCacheable() { @Test public void testResponsesWithMultipleExpiresHeadersAreNotCacheableUsingSharedPublicCache() { - policy = new ResponseCachingPolicy(true, false, false, true); + policy = new ResponseCachingPolicy(true, false, false); request.setHeader("Authorization", StandardAuthScheme.BASIC + " QWxhZGRpbjpvcGVuIHNlc2FtZQ=="); response.addHeader("Expires", DateUtils.formatStandardDate(now)); @@ -515,14 +515,14 @@ public void testResponsesToHEADWithQueryParamsButNoExplicitCachingAreNotCacheabl @Test public void testResponsesToGETWithQueryParamsButNoExplicitCachingAreNotCacheableEvenWhen1_0QueryCachingDisabled() { - policy = new ResponseCachingPolicy(true, true, false, false); + policy = new ResponseCachingPolicy(true, true, false); request = new BasicHttpRequest("GET", "/foo?s=bar"); Assertions.assertFalse(policy.isResponseCacheable(responseCacheControl, request, response)); } @Test public void testResponsesToHEADWithQueryParamsButNoExplicitCachingAreNotCacheableEvenWhen1_0QueryCachingDisabled() { - policy = new ResponseCachingPolicy(true, true, false, false); + policy = new ResponseCachingPolicy(true, true, false); request = new BasicHttpRequest("HEAD", "/foo?s=bar"); Assertions.assertFalse(policy.isResponseCacheable(responseCacheControl, request, response)); } @@ -537,7 +537,7 @@ public void testResponsesToGETWithQueryParamsAndExplicitCachingAreCacheable() { @Test public void testResponsesToHEADWithQueryParamsAndExplicitCachingAreCacheable() { - policy = new ResponseCachingPolicy(true, false, false, true); + policy = new ResponseCachingPolicy(true, false, false); request = new BasicHttpRequest("HEAD", "/foo?s=bar"); response.setHeader("Date", DateUtils.formatStandardDate(now)); response.setHeader("Expires", DateUtils.formatStandardDate(tenSecondsFromNow)); @@ -546,7 +546,7 @@ public void testResponsesToHEADWithQueryParamsAndExplicitCachingAreCacheable() { @Test public void testResponsesToGETWithQueryParamsAndExplicitCachingAreCacheableEvenWhen1_0QueryCachingDisabled() { - policy = new ResponseCachingPolicy(true, true, false, true); + policy = new ResponseCachingPolicy(true, true, false); request = new BasicHttpRequest("GET", "/foo?s=bar"); response.setHeader("Date", DateUtils.formatStandardDate(now)); response.setHeader("Expires", DateUtils.formatStandardDate(tenSecondsFromNow)); @@ -555,7 +555,7 @@ public void testResponsesToGETWithQueryParamsAndExplicitCachingAreCacheableEvenW @Test public void testResponsesToHEADWithQueryParamsAndExplicitCachingAreCacheableEvenWhen1_0QueryCachingDisabled() { - policy = new ResponseCachingPolicy(true, true, false, true); + policy = new ResponseCachingPolicy(true, true, false); request = new BasicHttpRequest("HEAD", "/foo?s=bar"); response.setHeader("Date", DateUtils.formatStandardDate(now)); response.setHeader("Expires", DateUtils.formatStandardDate(tenSecondsFromNow)); @@ -580,7 +580,7 @@ public void headsWithQueryParametersDirectlyFrom1_0OriginsAreNotCacheable() { @Test public void getsWithQueryParametersDirectlyFrom1_0OriginsAreNotCacheableEvenWithSetting() { - policy = new ResponseCachingPolicy(true, true, false, true); + policy = new ResponseCachingPolicy(true, true, false); request = new BasicHttpRequest("GET", "/foo?s=bar"); response = new BasicHttpResponse(HttpStatus.SC_OK, "OK"); response.setVersion(HttpVersion.HTTP_1_0); @@ -589,7 +589,7 @@ public void getsWithQueryParametersDirectlyFrom1_0OriginsAreNotCacheableEvenWith @Test public void headsWithQueryParametersDirectlyFrom1_0OriginsAreNotCacheableEvenWithSetting() { - policy = new ResponseCachingPolicy(true, true, false, true); + policy = new ResponseCachingPolicy(true, true, false); request = new BasicHttpRequest("HEAD", "/foo?s=bar"); response = new BasicHttpResponse(HttpStatus.SC_OK, "OK"); response.setVersion(HttpVersion.HTTP_1_0); @@ -608,7 +608,7 @@ public void getsWithQueryParametersDirectlyFrom1_0OriginsAreCacheableWithExpires @Test public void headsWithQueryParametersDirectlyFrom1_0OriginsAreCacheableWithExpires() { - policy = new ResponseCachingPolicy(true, false, false, true); + policy = new ResponseCachingPolicy(true, false, false); request = new BasicHttpRequest("HEAD", "/foo?s=bar"); response = new BasicHttpResponse(HttpStatus.SC_OK, "OK"); response.setVersion(HttpVersion.HTTP_1_0); @@ -619,7 +619,7 @@ public void headsWithQueryParametersDirectlyFrom1_0OriginsAreCacheableWithExpire @Test public void getsWithQueryParametersDirectlyFrom1_0OriginsCanBeNotCacheableEvenWithExpires() { - policy = new ResponseCachingPolicy(true, true, false, true); + policy = new ResponseCachingPolicy(true, true, false); request = new BasicHttpRequest("GET", "/foo?s=bar"); response = new BasicHttpResponse(HttpStatus.SC_OK, "OK"); response.setVersion(HttpVersion.HTTP_1_0); @@ -630,7 +630,7 @@ public void getsWithQueryParametersDirectlyFrom1_0OriginsCanBeNotCacheableEvenWi @Test public void headsWithQueryParametersDirectlyFrom1_0OriginsCanBeNotCacheableEvenWithExpires() { - policy = new ResponseCachingPolicy(true, true, false, true); + policy = new ResponseCachingPolicy(true, true, false); request = new BasicHttpRequest("HEAD", "/foo?s=bar"); response = new BasicHttpResponse(HttpStatus.SC_OK, "OK"); response.setVersion(HttpVersion.HTTP_1_0); @@ -664,7 +664,7 @@ public void getsWithQueryParametersFrom1_0OriginsViaProxiesAreCacheableWithExpir @Test public void headsWithQueryParametersFrom1_0OriginsViaProxiesAreCacheableWithExpires() { - policy = new ResponseCachingPolicy(true, false, false, true); + policy = new ResponseCachingPolicy(true, false, false); request = new BasicHttpRequest("HEAD", "/foo?s=bar"); response.setHeader("Date", DateUtils.formatStandardDate(now)); response.setHeader("Expires", DateUtils.formatStandardDate(tenSecondsFromNow)); @@ -674,7 +674,7 @@ public void headsWithQueryParametersFrom1_0OriginsViaProxiesAreCacheableWithExpi @Test public void getsWithQueryParametersFrom1_0OriginsViaProxiesCanNotBeCacheableEvenWithExpires() { - policy = new ResponseCachingPolicy(true, true, true, true); + policy = new ResponseCachingPolicy(true, true, true); request = new BasicHttpRequest("GET", "/foo?s=bar"); response.setHeader("Date", DateUtils.formatStandardDate(now)); response.setHeader("Expires", DateUtils.formatStandardDate(tenSecondsFromNow)); @@ -684,7 +684,7 @@ public void getsWithQueryParametersFrom1_0OriginsViaProxiesCanNotBeCacheableEven @Test public void headsWithQueryParametersFrom1_0OriginsViaProxiesCanNotBeCacheableEvenWithExpires() { - policy = new ResponseCachingPolicy(true, true, true, true); + policy = new ResponseCachingPolicy(true, true, true); request = new BasicHttpRequest("HEAD", "/foo?s=bar"); response.setHeader("Date", DateUtils.formatStandardDate(now)); response.setHeader("Expires", DateUtils.formatStandardDate(tenSecondsFromNow)); @@ -703,7 +703,7 @@ public void getsWithQueryParametersFrom1_0OriginsViaExplicitProxiesAreCacheableW @Test public void headsWithQueryParametersFrom1_0OriginsViaExplicitProxiesAreCacheableWithExpires() { - policy = new ResponseCachingPolicy(true, false, false, false); + policy = new ResponseCachingPolicy(true, false, false); request = new BasicHttpRequest("HEAD", "/foo?s=bar"); response.setHeader("Date", DateUtils.formatStandardDate(now)); response.setHeader("Expires", DateUtils.formatStandardDate(tenSecondsFromNow)); @@ -713,7 +713,7 @@ public void headsWithQueryParametersFrom1_0OriginsViaExplicitProxiesAreCacheable @Test public void getsWithQueryParametersFrom1_0OriginsViaExplicitProxiesCanNotBeCacheableEvenWithExpires() { - policy = new ResponseCachingPolicy(true, true, true, true); + policy = new ResponseCachingPolicy(true, true, true); request = new BasicHttpRequest("GET", "/foo?s=bar"); response.setHeader("Date", DateUtils.formatStandardDate(now)); response.setHeader("Expires", DateUtils.formatStandardDate(tenSecondsFromNow)); @@ -723,7 +723,7 @@ public void getsWithQueryParametersFrom1_0OriginsViaExplicitProxiesCanNotBeCache @Test public void headsWithQueryParametersFrom1_0OriginsViaExplicitProxiesCanNotBeCacheableEvenWithExpires() { - policy = new ResponseCachingPolicy(true, true, true, true); + policy = new ResponseCachingPolicy(true, true, true); request = new BasicHttpRequest("HEAD", "/foo?s=bar"); response.setHeader("Date", DateUtils.formatStandardDate(now)); response.setHeader("Expires", DateUtils.formatStandardDate(tenSecondsFromNow)); @@ -744,7 +744,7 @@ public void getsWithQueryParametersFrom1_1OriginsVia1_0ProxiesAreCacheableWithEx @Test public void headsWithQueryParametersFrom1_1OriginsVia1_0ProxiesAreCacheableWithExpires() { - policy = new ResponseCachingPolicy(true, false, false, true); + policy = new ResponseCachingPolicy(true, false, false); request = new BasicHttpRequest("HEAD", "/foo?s=bar"); response = new BasicHttpResponse(HttpStatus.SC_OK, "OK"); response.setVersion(HttpVersion.HTTP_1_0); @@ -783,7 +783,7 @@ public void test302WithExplicitCachingHeaders() { public void test303WithExplicitCachingHeadersWhenPermittedByConfig() { // HTTPbis working group says ok if explicitly indicated by // response headers - policy = new ResponseCachingPolicy(true, false, true, true); + policy = new ResponseCachingPolicy(true, false, true); response.setCode(HttpStatus.SC_SEE_OTHER); response.setHeader("Date", DateUtils.formatStandardDate(now)); responseCacheControl = ResponseCacheControl.builder() @@ -824,7 +824,7 @@ void testIsResponseCacheableNullCacheControl() { // Create ResponseCachingPolicy instance and test the method - policy = new ResponseCachingPolicy(true, false, false, false); + policy = new ResponseCachingPolicy(true, false, false); request = new BasicHttpRequest("GET", "/foo"); assertTrue(policy.isResponseCacheable(responseCacheControl, request, response)); } @@ -842,7 +842,7 @@ void testIsResponseCacheableNotNullCacheControlSmaxAge60() { // Create ResponseCachingPolicy instance and test the method - policy = new ResponseCachingPolicy(true, false, false, false); + policy = new ResponseCachingPolicy(true, false, false); request = new BasicHttpRequest("GET", "/foo"); responseCacheControl = ResponseCacheControl.builder() .setMaxAge(60) @@ -862,7 +862,7 @@ void testIsResponseCacheableNotNullCacheControlMaxAge60() { // Create ResponseCachingPolicy instance and test the method - policy = new ResponseCachingPolicy(true, false, false,false); + policy = new ResponseCachingPolicy(true, false, false); request = new BasicHttpRequest("GET", "/foo"); responseCacheControl = ResponseCacheControl.builder() .setMaxAge(60) @@ -882,7 +882,7 @@ void testIsResponseCacheableNotExsiresAndDate() { // Create ResponseCachingPolicy instance and test the method - policy = new ResponseCachingPolicy(true, false, false,false); + policy = new ResponseCachingPolicy(true, false, false); request = new BasicHttpRequest("GET", "/foo"); assertTrue(policy.isResponseCacheable(responseCacheControl, request, response)); } @@ -898,7 +898,7 @@ public void testIsResponseCacheable() { request = new BasicHttpRequest("GET","/foo?s=bar"); // HTTPbis working group says ok if explicitly indicated by // response headers - policy = new ResponseCachingPolicy(true, false, true, true); + policy = new ResponseCachingPolicy(true, false, true); response.setCode(HttpStatus.SC_OK); response.setHeader("Date", DateUtils.formatStandardDate(now)); assertTrue(policy.isResponseCacheable(responseCacheControl, request, response)); @@ -911,7 +911,7 @@ void testIsResponseCacheableNoCache() { response.setHeader(HttpHeaders.DATE, DateUtils.formatStandardDate(Instant.now())); // Create ResponseCachingPolicy instance and test the method - policy = new ResponseCachingPolicy(true, false, false, true); + policy = new ResponseCachingPolicy(true, false, false); request = new BasicHttpRequest("GET", "/foo"); responseCacheControl = ResponseCacheControl.builder() .setNoCache(true) @@ -926,7 +926,7 @@ void testIsResponseCacheableNoStore() { response.setHeader(HttpHeaders.DATE, DateUtils.formatStandardDate(Instant.now())); // Create ResponseCachingPolicy instance and test the method - policy = new ResponseCachingPolicy(true, false, false, false); + policy = new ResponseCachingPolicy(true, false, false); request = new BasicHttpRequest("GET", "/foo"); responseCacheControl = ResponseCacheControl.builder() .setNoStore(true) From ebd2c678d6b85bffad7e00f80b9d57d22e8778df Mon Sep 17 00:00:00 2001 From: Oleg Kalnichevski Date: Sat, 11 Nov 2023 20:23:45 +0100 Subject: [PATCH 6/8] Better HTTP execution context management by caching protocol handlers --- .../http/impl/cache/AsyncCachingExec.java | 255 +++++++++++------- .../client5/http/impl/cache/CachingExec.java | 129 +++++---- .../http/impl/cache/CachingExecBase.java | 56 +--- .../http/impl/cache/TestCachingExecChain.java | 20 +- 4 files changed, 247 insertions(+), 213 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 2440c3d809..570136bb7c 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 @@ -47,6 +47,7 @@ import org.apache.hc.client5.http.async.methods.SimpleHttpResponse; import org.apache.hc.client5.http.cache.CacheResponseStatus; import org.apache.hc.client5.http.cache.HttpAsyncCacheStorage; +import org.apache.hc.client5.http.cache.HttpCacheContext; import org.apache.hc.client5.http.cache.HttpCacheEntry; import org.apache.hc.client5.http.cache.ResourceFactory; import org.apache.hc.client5.http.cache.ResourceIOException; @@ -106,22 +107,6 @@ class AsyncCachingExec extends CachingExecBase implements AsyncExecChainHandler BasicRequestBuilder.copy(request).build()); } - AsyncCachingExec( - final HttpAsyncCache responseCache, - final CacheValidityPolicy validityPolicy, - final ResponseCachingPolicy responseCachingPolicy, - final CachedHttpResponseGenerator responseGenerator, - final CacheableRequestPolicy cacheableRequestPolicy, - final CachedResponseSuitabilityChecker suitabilityChecker, - final DefaultAsyncCacheRevalidator cacheRevalidator, - final ConditionalRequestBuilder conditionalRequestBuilder, - final CacheConfig config) { - super(validityPolicy, responseCachingPolicy, responseGenerator, cacheableRequestPolicy, suitabilityChecker, config); - this.responseCache = responseCache; - this.cacheRevalidator = cacheRevalidator; - this.conditionalRequestBuilder = conditionalRequestBuilder; - } - AsyncCachingExec( final HttpAsyncCache cache, final ScheduledExecutorService executorService, @@ -145,7 +130,6 @@ private void triggerResponse( final SimpleHttpResponse cacheResponse, final AsyncExecChain.Scope scope, final AsyncExecCallback asyncExecCallback) { - scope.clientContext.setAttribute(HttpCoreContext.HTTP_RESPONSE, cacheResponse); scope.execRuntime.releaseEndpoint(); final SimpleBody body = cacheResponse.getBody(); @@ -213,20 +197,61 @@ public void execute( Args.notNull(scope, "Scope"); final HttpRoute route = scope.route; - final CancellableDependency operation = scope.cancellableDependency; final HttpClientContext context = scope.clientContext; - context.setAttribute(HttpClientContext.HTTP_ROUTE, route); - context.setAttribute(HttpCoreContext.HTTP_REQUEST, request); final URIAuthority authority = request.getAuthority(); final String scheme = request.getScheme(); final HttpHost target = authority != null ? new HttpHost(scheme, authority) : route.getTargetHost(); + doExecute(target, + request, + entityProducer, + scope, + chain, + new AsyncExecCallback() { - // default response context - setResponseStatus(context, CacheResponseStatus.CACHE_MISS); + @Override + public AsyncDataConsumer handleResponse( + final HttpResponse response, + final EntityDetails entityDetails) throws HttpException, IOException { + context.setAttribute(HttpCoreContext.HTTP_REQUEST, request); + context.setAttribute(HttpCoreContext.HTTP_RESPONSE, response); + return asyncExecCallback.handleResponse(response, entityDetails); + } + + @Override + public void handleInformationResponse( + final HttpResponse response) throws HttpException, IOException { + asyncExecCallback.handleInformationResponse(response); + } + + @Override + public void completed() { + asyncExecCallback.completed(); + } + + @Override + public void failed(final Exception cause) { + asyncExecCallback.failed(cause); + } + + }); + } + + public void doExecute( + final HttpHost target, + final HttpRequest request, + final AsyncEntityProducer entityProducer, + final AsyncExecChain.Scope scope, + final AsyncExecChain chain, + final AsyncExecCallback asyncExecCallback) throws HttpException, IOException { + + final HttpClientContext context = scope.clientContext; + final CancellableDependency operation = scope.cancellableDependency; + + context.setAttribute(HttpCacheContext.CACHE_RESPONSE_STATUS, CacheResponseStatus.CACHE_MISS); if (clientRequestsOurOptions(request)) { - setResponseStatus(context, CacheResponseStatus.CACHE_MODULE_RESPONSE); + context.setAttribute(HttpCacheContext.CACHE_RESPONSE_STATUS, CacheResponseStatus.CACHE_MODULE_RESPONSE); triggerResponse(SimpleHttpResponse.create(HttpStatus.SC_NOT_IMPLEMENTED), scope, asyncExecCallback); return; } @@ -596,7 +621,12 @@ private void handleCacheHit( final AsyncExecChain chain, final AsyncExecCallback asyncExecCallback) { final HttpClientContext context = scope.clientContext; - recordCacheHit(target, request); + if (LOG.isDebugEnabled()) { + LOG.debug("Request {} {}: cache hit", request.getMethod(), request.getRequestUri()); + } + context.setAttribute(HttpCacheContext.CACHE_RESPONSE_STATUS, CacheResponseStatus.CACHE_HIT); + cacheHits.getAndIncrement(); + final Instant now = getCurrentDate(); final CacheSuitability cacheSuitability = suitabilityChecker.assessSuitability(requestCacheControl, responseCacheControl, request, hit.entry, now); @@ -604,17 +634,17 @@ private void handleCacheHit( LOG.debug("Request {} {}: {}", request.getMethod(), request.getRequestUri(), cacheSuitability); } if (cacheSuitability == CacheSuitability.FRESH || cacheSuitability == CacheSuitability.FRESH_ENOUGH) { - LOG.debug("Cache hit"); + LOG.debug("Cache hit is suitable"); try { - final SimpleHttpResponse cacheResponse = generateCachedResponse(request, context, hit.entry); + final SimpleHttpResponse cacheResponse = generateCachedResponse(request, hit.entry, now); triggerResponse(cacheResponse, scope, asyncExecCallback); } catch (final ResourceIOException ex) { - recordCacheFailure(target, request); if (!mayCallBackend(requestCacheControl)) { - final SimpleHttpResponse cacheResponse = generateGatewayTimeout(context); + context.setAttribute(HttpCacheContext.CACHE_RESPONSE_STATUS, CacheResponseStatus.CACHE_MODULE_RESPONSE); + final SimpleHttpResponse cacheResponse = generateGatewayTimeout(); triggerResponse(cacheResponse, scope, asyncExecCallback); } else { - setResponseStatus(scope.clientContext, CacheResponseStatus.FAILURE); + context.setAttribute(HttpCacheContext.CACHE_RESPONSE_STATUS, CacheResponseStatus.FAILURE); try { chain.proceed(request, entityProducer, scope, asyncExecCallback); } catch (final HttpException | IOException ex2) { @@ -625,7 +655,8 @@ private void handleCacheHit( } else { if (!mayCallBackend(requestCacheControl)) { LOG.debug("Cache entry not is not fresh and only-if-cached requested"); - final SimpleHttpResponse cacheResponse = generateGatewayTimeout(context); + context.setAttribute(HttpCacheContext.CACHE_RESPONSE_STATUS, CacheResponseStatus.CACHE_MODULE_RESPONSE); + final SimpleHttpResponse cacheResponse = generateGatewayTimeout(); triggerResponse(cacheResponse, scope, asyncExecCallback); } else if (cacheSuitability == CacheSuitability.MISMATCH) { LOG.debug("Cache entry does not match the request; calling backend"); @@ -638,39 +669,7 @@ private void handleCacheHit( callBackend(target, request, entityProducer, scope, chain, asyncExecCallback); } else if (cacheSuitability == CacheSuitability.REVALIDATION_REQUIRED) { LOG.debug("Revalidation required; revalidating cache entry"); - revalidateCacheEntry(responseCacheControl, hit, target, request, entityProducer, scope, chain, new AsyncExecCallback() { - - private final AtomicBoolean committed = new AtomicBoolean(); - - @Override - public AsyncDataConsumer handleResponse(final HttpResponse response, - final EntityDetails entityDetails) throws HttpException, IOException { - committed.set(true); - return asyncExecCallback.handleResponse(response, entityDetails); - } - - @Override - public void handleInformationResponse(final HttpResponse response) throws HttpException, IOException { - asyncExecCallback.handleInformationResponse(response); - } - - @Override - public void completed() { - asyncExecCallback.completed(); - } - - @Override - public void failed(final Exception cause) { - if (!committed.get() && cause instanceof IOException) { - final SimpleHttpResponse cacheResponse = generateGatewayTimeout(scope.clientContext); - LOG.debug(cause.getMessage(), cause); - triggerResponse(cacheResponse, scope, asyncExecCallback); - } else { - asyncExecCallback.failed(cause); - } - } - - }); + revalidateCacheEntryWithoutFallback(responseCacheControl, hit, target, request, entityProducer, scope, chain, asyncExecCallback); } else if (cacheSuitability == CacheSuitability.STALE_WHILE_REVALIDATED) { if (cacheRevalidator != null) { LOG.debug("Serving stale with asynchronous revalidation"); @@ -690,7 +689,8 @@ public void failed(final Exception cause) { hit.getEntryKey(), asyncExecCallback, c -> revalidateCacheEntry(responseCacheControl, hit, target, request, entityProducer, fork, chain, c)); - final SimpleHttpResponse cacheResponse = unvalidatedCacheHit(request, context, hit.entry); + context.setAttribute(HttpCacheContext.CACHE_RESPONSE_STATUS, CacheResponseStatus.CACHE_MODULE_RESPONSE); + final SimpleHttpResponse cacheResponse = unvalidatedCacheHit(request, hit.entry); triggerResponse(cacheResponse, scope, asyncExecCallback); } catch (final IOException ex) { asyncExecCallback.failed(ex); @@ -723,13 +723,13 @@ void revalidateCacheEntry( responseCacheControl, BasicRequestBuilder.copy(scope.originalRequest).build(), hit.entry); + final HttpClientContext context = scope.clientContext; chainProceed(conditionalRequest, entityProducer, scope, chain, new AsyncExecCallback() { final AtomicReference callbackRef = new AtomicReference<>(); void triggerUpdatedCacheEntryResponse(final HttpResponse backendResponse, final Instant responseDate) { final CancellableDependency operation = scope.cancellableDependency; - recordCacheUpdate(scope.clientContext); operation.setDependency(responseCache.update( hit, target, @@ -742,7 +742,7 @@ void triggerUpdatedCacheEntryResponse(final HttpResponse backendResponse, final @Override public void completed(final CacheHit updated) { try { - final SimpleHttpResponse cacheResponse = generateCachedResponse(request, scope.clientContext, updated.entry); + final SimpleHttpResponse cacheResponse = generateCachedResponse(request, updated.entry, responseDate); triggerResponse(cacheResponse, scope, asyncExecCallback); } catch (final ResourceIOException ex) { asyncExecCallback.failed(ex); @@ -762,19 +762,11 @@ public void cancelled() { })); } - void triggerResponseStaleCacheEntry() { - try { - final SimpleHttpResponse cacheResponse = responseGenerator.generateResponse(request, hit.entry); - triggerResponse(cacheResponse, scope, asyncExecCallback); - } catch (final ResourceIOException ex) { - asyncExecCallback.failed(ex); - } - } - AsyncExecCallback evaluateResponse(final HttpResponse backendResponse, final Instant responseDate) { final int statusCode = backendResponse.getCode(); if (statusCode == HttpStatus.SC_NOT_MODIFIED || statusCode == HttpStatus.SC_OK) { - recordCacheUpdate(scope.clientContext); + context.setAttribute(HttpCacheContext.CACHE_RESPONSE_STATUS, CacheResponseStatus.VALIDATED); + cacheUpdates.getAndIncrement(); } if (statusCode == HttpStatus.SC_NOT_MODIFIED) { return new AsyncExecCallbackWrapper(() -> triggerUpdatedCacheEntryResponse(backendResponse, responseDate), asyncExecCallback::failed); @@ -787,7 +779,7 @@ public AsyncDataConsumer handleResponse( final HttpResponse backendResponse1, final EntityDetails entityDetails) throws HttpException, IOException { - final Instant responseDate1 = getCurrentDate(); + final Instant responseDate = getCurrentDate(); final AsyncExecCallback callback1; if (HttpCacheEntry.isNewer(hit.entry, backendResponse1)) { @@ -839,7 +831,7 @@ public void failed(final Exception cause) { }), asyncExecCallback::failed); } else { - callback1 = evaluateResponse(backendResponse1, responseDate1); + callback1 = evaluateResponse(backendResponse1, responseDate); } callbackRef.set(callback1); return callback1.handleResponse(backendResponse1, entityDetails); @@ -879,6 +871,52 @@ public void failed(final Exception cause) { } + void revalidateCacheEntryWithoutFallback( + final ResponseCacheControl responseCacheControl, + final CacheHit hit, + final HttpHost target, + final HttpRequest request, + final AsyncEntityProducer entityProducer, + final AsyncExecChain.Scope scope, + final AsyncExecChain chain, + final AsyncExecCallback asyncExecCallback) { + final HttpClientContext context = scope.clientContext; + revalidateCacheEntry(responseCacheControl, hit, target, request, entityProducer, scope, chain, new AsyncExecCallback() { + + private final AtomicBoolean committed = new AtomicBoolean(); + + @Override + public AsyncDataConsumer handleResponse(final HttpResponse response, + final EntityDetails entityDetails) throws HttpException, IOException { + committed.set(true); + return asyncExecCallback.handleResponse(response, entityDetails); + } + + @Override + public void handleInformationResponse(final HttpResponse response) throws HttpException, IOException { + asyncExecCallback.handleInformationResponse(response); + } + + @Override + public void completed() { + asyncExecCallback.completed(); + } + + @Override + public void failed(final Exception cause) { + if (!committed.get() && cause instanceof IOException) { + LOG.debug(cause.getMessage(), cause); + final SimpleHttpResponse cacheResponse = generateGatewayTimeout(); + context.setAttribute(HttpCacheContext.CACHE_RESPONSE_STATUS, CacheResponseStatus.CACHE_MODULE_RESPONSE); + triggerResponse(cacheResponse, scope, asyncExecCallback); + } else { + asyncExecCallback.failed(cause); + } + } + + }); + } + void revalidateCacheEntryWithFallback( final RequestCacheControl requestCacheControl, final ResponseCacheControl responseCacheControl, @@ -889,18 +927,22 @@ void revalidateCacheEntryWithFallback( final AsyncExecChain.Scope scope, final AsyncExecChain chain, final AsyncExecCallback asyncExecCallback) { + final HttpClientContext context = scope.clientContext; revalidateCacheEntry(responseCacheControl, hit, target, request, entityProducer, scope, chain, new AsyncExecCallback() { - private final AtomicBoolean committed = new AtomicBoolean(); + private final AtomicReference committed = new AtomicReference<>(); @Override public AsyncDataConsumer handleResponse(final HttpResponse response, final EntityDetails entityDetails) throws HttpException, IOException { final int status = response.getCode(); if (staleIfErrorAppliesTo(status) && suitabilityChecker.isSuitableIfError(requestCacheControl, responseCacheControl, hit.entry, getCurrentDate())) { + if (LOG.isDebugEnabled()) { + LOG.debug("Serving stale response due to {} status and stale-if-error enabled", status); + } return null; } else { - committed.set(true); + committed.set(response); return asyncExecCallback.handleResponse(response, entityDetails); } } @@ -912,33 +954,41 @@ public void handleInformationResponse(final HttpResponse response) throws HttpEx @Override public void completed() { - if (committed.get()) { - asyncExecCallback.completed(); - } else { + final HttpResponse response = committed.get(); + if (response == null) { try { - final SimpleHttpResponse cacheResponse = unvalidatedCacheHit(request, scope.clientContext, hit.entry); + context.setAttribute(HttpCacheContext.CACHE_RESPONSE_STATUS, CacheResponseStatus.CACHE_MODULE_RESPONSE); + final SimpleHttpResponse cacheResponse = unvalidatedCacheHit(request, hit.entry); triggerResponse(cacheResponse, scope, asyncExecCallback); } catch (final IOException ex) { asyncExecCallback.failed(ex); } + } else { + asyncExecCallback.completed(); } } @Override public void failed(final Exception cause) { - if (!committed.get() && - cause instanceof IOException && - suitabilityChecker.isSuitableIfError(requestCacheControl, responseCacheControl, hit.entry, getCurrentDate())) { - try { - final SimpleHttpResponse cacheResponse = unvalidatedCacheHit(request, scope.clientContext, hit.entry); + final HttpResponse response = committed.get(); + if (response == null) { + LOG.debug(cause.getMessage(), cause); + context.setAttribute(HttpCacheContext.CACHE_RESPONSE_STATUS, CacheResponseStatus.CACHE_MODULE_RESPONSE); + if (cause instanceof IOException && + suitabilityChecker.isSuitableIfError(requestCacheControl, responseCacheControl, hit.entry, getCurrentDate())) { + LOG.debug("Serving stale response due to IOException and stale-if-error enabled"); + try { + final SimpleHttpResponse cacheResponse = unvalidatedCacheHit(request, hit.entry); + triggerResponse(cacheResponse, scope, asyncExecCallback); + } catch (final IOException ex) { + asyncExecCallback.failed(cause); + } + } else { + final SimpleHttpResponse cacheResponse = generateGatewayTimeout(); triggerResponse(cacheResponse, scope, asyncExecCallback); - } catch (final IOException ex) { - asyncExecCallback.failed(cause); } } else { - LOG.debug(cause.getMessage(), cause); - final SimpleHttpResponse cacheResponse = generateGatewayTimeout(scope.clientContext); - triggerResponse(cacheResponse, scope, asyncExecCallback); + asyncExecCallback.failed(cause); } } @@ -953,11 +1003,16 @@ private void handleCacheMiss( final AsyncExecChain.Scope scope, final AsyncExecChain chain, final AsyncExecCallback asyncExecCallback) { - recordCacheMiss(target, request); + cacheMisses.getAndIncrement(); + if (LOG.isDebugEnabled()) { + LOG.debug("Request {} {}: cache miss", request.getMethod(), request.getRequestUri()); + } final CancellableDependency operation = scope.cancellableDependency; if (!mayCallBackend(requestCacheControl)) { - final SimpleHttpResponse cacheResponse = SimpleHttpResponse.create(HttpStatus.SC_GATEWAY_TIMEOUT, "Gateway Timeout"); + final HttpClientContext context = scope.clientContext; + context.setAttribute(HttpCacheContext.CACHE_RESPONSE_STATUS, CacheResponseStatus.CACHE_MODULE_RESPONSE); + final SimpleHttpResponse cacheResponse = generateGatewayTimeout(); triggerResponse(cacheResponse, scope, asyncExecCallback); } @@ -1017,7 +1072,10 @@ void negotiateResponseFromVariants( final AtomicReference callbackRef = new AtomicReference<>(); void updateVariantCacheEntry(final HttpResponse backendResponse, final Instant responseDate, final CacheHit match) { - recordCacheUpdate(scope.clientContext); + final HttpClientContext context = scope.clientContext; + context.setAttribute(HttpCacheContext.CACHE_RESPONSE_STATUS, CacheResponseStatus.VALIDATED); + cacheUpdates.getAndIncrement(); + operation.setDependency(responseCache.storeFromNegotiated( match, target, @@ -1141,8 +1199,7 @@ public void cancelled() { if (HttpCacheEntry.isNewer(match.entry, backendResponse)) { final HttpRequest unconditional = conditionalRequestBuilder.buildUnconditionalRequest( BasicRequestBuilder.copy(request).build()); - scope.clientContext.setAttribute(HttpCoreContext.HTTP_REQUEST, unconditional); - callback = new AsyncExecCallbackWrapper(() -> callBackend(target, request, entityProducer, scope, chain, asyncExecCallback), asyncExecCallback::failed); + callback = new AsyncExecCallbackWrapper(() -> callBackend(target, unconditional, entityProducer, scope, chain, asyncExecCallback), asyncExecCallback::failed); } else { callback = new AsyncExecCallbackWrapper(() -> updateVariantCacheEntry(backendResponse, responseDate, match), asyncExecCallback::failed); } 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 5e1f4bb143..6b21bb5011 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 @@ -39,6 +39,7 @@ import org.apache.hc.client5.http.async.methods.SimpleBody; import org.apache.hc.client5.http.async.methods.SimpleHttpResponse; 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; @@ -150,18 +151,30 @@ public ClassicHttpResponse execute( final HttpRoute route = scope.route; final HttpClientContext context = scope.clientContext; - context.setAttribute(HttpClientContext.HTTP_ROUTE, scope.route); - context.setAttribute(HttpCoreContext.HTTP_REQUEST, request); final URIAuthority authority = request.getAuthority(); final String scheme = request.getScheme(); final HttpHost target = authority != null ? new HttpHost(scheme, authority) : route.getTargetHost(); + final ClassicHttpResponse response = doExecute(target, request, scope, chain); + + context.setAttribute(HttpCoreContext.HTTP_REQUEST, request); + context.setAttribute(HttpCoreContext.HTTP_RESPONSE, response); + + return response; + } - // default response context - setResponseStatus(context, CacheResponseStatus.CACHE_MISS); + ClassicHttpResponse doExecute( + final HttpHost target, + final ClassicHttpRequest request, + final ExecChain.Scope scope, + final ExecChain chain) throws IOException, HttpException { + + final HttpClientContext context = scope.clientContext; + + context.setAttribute(HttpCacheContext.CACHE_RESPONSE_STATUS, CacheResponseStatus.CACHE_MISS); if (clientRequestsOurOptions(request)) { - setResponseStatus(context, CacheResponseStatus.CACHE_MODULE_RESPONSE); + context.setAttribute(HttpCacheContext.CACHE_RESPONSE_STATUS, CacheResponseStatus.CACHE_MODULE_RESPONSE); return new BasicClassicHttpResponse(HttpStatus.SC_NOT_IMPLEMENTED); } final CacheMatch result = responseCache.match(target, request); @@ -177,7 +190,6 @@ public ClassicHttpResponse execute( return callBackend(target, request, scope, chain); } - if (hit == null) { LOG.debug("Cache miss"); return handleCacheMiss(requestCacheControl, root, target, request, scope, chain); @@ -190,7 +202,7 @@ public ClassicHttpResponse execute( } } - private static ClassicHttpResponse convert(final SimpleHttpResponse cacheResponse, final ExecChain.Scope scope) { + private static ClassicHttpResponse convert(final SimpleHttpResponse cacheResponse) { if (cacheResponse == null) { return null; } @@ -210,7 +222,6 @@ private static ClassicHttpResponse convert(final SimpleHttpResponse cacheRespons response.setEntity(new ByteArrayEntity(body.getBodyBytes(), contentType, contentEncoding, false)); } } - scope.clientContext.setAttribute(HttpCoreContext.HTTP_RESPONSE, response); return response; } @@ -225,7 +236,7 @@ ClassicHttpResponse callBackend( LOG.debug("Calling the backend"); final ClassicHttpResponse backendResponse = chain.proceed(request, scope); try { - return handleBackendResponse(target, request, scope, requestDate, getCurrentDate(), backendResponse); + return handleBackendResponse(target, request, requestDate, getCurrentDate(), backendResponse); } catch (final IOException | RuntimeException ex) { backendResponse.close(); throw ex; @@ -241,8 +252,12 @@ private ClassicHttpResponse handleCacheHit( final ExecChain.Scope scope, final ExecChain chain) throws IOException, HttpException { final HttpClientContext context = scope.clientContext; - context.setAttribute(HttpCoreContext.HTTP_REQUEST, request); - recordCacheHit(target, request); + if (LOG.isDebugEnabled()) { + LOG.debug("Request {} {}: cache hit", request.getMethod(), request.getRequestUri()); + } + context.setAttribute(HttpCacheContext.CACHE_RESPONSE_STATUS, CacheResponseStatus.CACHE_HIT); + cacheHits.getAndIncrement(); + final Instant now = getCurrentDate(); final CacheSuitability cacheSuitability = suitabilityChecker.assessSuitability(requestCacheControl, responseCacheControl, request, hit.entry, now); @@ -250,21 +265,22 @@ private ClassicHttpResponse handleCacheHit( LOG.debug("Request {} {}: {}", request.getMethod(), request.getRequestUri(), cacheSuitability); } if (cacheSuitability == CacheSuitability.FRESH || cacheSuitability == CacheSuitability.FRESH_ENOUGH) { - LOG.debug("Cache hit"); + LOG.debug("Cache hit is suitable"); try { - return convert(generateCachedResponse(request, context, hit.entry), scope); + return convert(generateCachedResponse(request, hit.entry, now)); } catch (final ResourceIOException ex) { - recordCacheFailure(target, request); if (!mayCallBackend(requestCacheControl)) { - return convert(generateGatewayTimeout(context), scope); + context.setAttribute(HttpCacheContext.CACHE_RESPONSE_STATUS, CacheResponseStatus.CACHE_MODULE_RESPONSE); + return convert(generateGatewayTimeout()); } - setResponseStatus(scope.clientContext, CacheResponseStatus.FAILURE); + context.setAttribute(HttpCacheContext.CACHE_RESPONSE_STATUS, CacheResponseStatus.FAILURE); return chain.proceed(request, scope); } } else { if (!mayCallBackend(requestCacheControl)) { LOG.debug("Cache entry not is not fresh and only-if-cached requested"); - return convert(generateGatewayTimeout(context), scope); + context.setAttribute(HttpCacheContext.CACHE_RESPONSE_STATUS, CacheResponseStatus.CACHE_MODULE_RESPONSE); + return convert(generateGatewayTimeout()); } else if (cacheSuitability == CacheSuitability.MISMATCH) { LOG.debug("Cache entry does not match the request; calling backend"); return callBackend(target, request, scope, chain); @@ -276,12 +292,7 @@ private ClassicHttpResponse handleCacheHit( return callBackend(target, request, scope, chain); } else if (cacheSuitability == CacheSuitability.REVALIDATION_REQUIRED) { LOG.debug("Revalidation required; revalidating cache entry"); - try { - return revalidateCacheEntry(responseCacheControl, hit, target, request, scope, chain); - } catch (final IOException ex) { - LOG.debug(ex.getMessage(), ex); - return convert(generateGatewayTimeout(scope.clientContext), scope); - } + return revalidateCacheEntryWithoutFallback(responseCacheControl, hit, target, request, scope, chain); } else if (cacheSuitability == CacheSuitability.STALE_WHILE_REVALIDATED) { if (cacheRevalidator != null) { LOG.debug("Serving stale with asynchronous revalidation"); @@ -296,8 +307,8 @@ private ClassicHttpResponse handleCacheHit( cacheRevalidator.revalidateCacheEntry( hit.getEntryKey(), () -> revalidateCacheEntry(responseCacheControl, hit, target, request, fork, chain)); - final SimpleHttpResponse response = unvalidatedCacheHit(request, context, hit.entry); - return convert(response, scope); + context.setAttribute(HttpCacheContext.CACHE_RESPONSE_STATUS, CacheResponseStatus.CACHE_MODULE_RESPONSE); + return convert(unvalidatedCacheHit(request, hit.entry)); } else { LOG.debug("Revalidating stale cache entry (asynchronous revalidation disabled)"); return revalidateCacheEntryWithFallback(requestCacheControl, responseCacheControl, hit, target, request, scope, chain); @@ -319,6 +330,7 @@ ClassicHttpResponse revalidateCacheEntry( final ClassicHttpRequest request, final ExecChain.Scope scope, final ExecChain chain) throws IOException, HttpException { + final HttpClientContext context = scope.clientContext; Instant requestDate = getCurrentDate(); final ClassicHttpRequest conditionalRequest = conditionalRequestBuilder.buildConditionalRequest( responseCacheControl, scope.originalRequest, hit.entry); @@ -338,19 +350,37 @@ ClassicHttpResponse revalidateCacheEntry( final int statusCode = backendResponse.getCode(); if (statusCode == HttpStatus.SC_NOT_MODIFIED || statusCode == HttpStatus.SC_OK) { - recordCacheUpdate(scope.clientContext); + context.setAttribute(HttpCacheContext.CACHE_RESPONSE_STATUS, CacheResponseStatus.VALIDATED); + cacheUpdates.getAndIncrement(); } if (statusCode == HttpStatus.SC_NOT_MODIFIED) { final CacheHit updated = responseCache.update(hit, target, request, backendResponse, requestDate, responseDate); - return convert(generateCachedResponse(request, scope.clientContext, updated.entry), scope); + return convert(generateCachedResponse(request, updated.entry, responseDate)); } - return handleBackendResponse(target, conditionalRequest, scope, requestDate, responseDate, backendResponse); + return handleBackendResponse(target, conditionalRequest, requestDate, responseDate, backendResponse); } catch (final IOException | RuntimeException ex) { backendResponse.close(); throw ex; } } + ClassicHttpResponse revalidateCacheEntryWithoutFallback( + final ResponseCacheControl responseCacheControl, + final CacheHit hit, + final HttpHost target, + final ClassicHttpRequest request, + final ExecChain.Scope scope, + final ExecChain chain) throws HttpException { + final HttpClientContext context = scope.clientContext; + try { + return revalidateCacheEntry(responseCacheControl, hit, target, request, scope, chain); + } catch (final IOException ex) { + LOG.debug(ex.getMessage(), ex); + context.setAttribute(HttpCacheContext.CACHE_RESPONSE_STATUS, CacheResponseStatus.CACHE_MODULE_RESPONSE); + return convert(generateGatewayTimeout()); + } + } + ClassicHttpResponse revalidateCacheEntryWithFallback( final RequestCacheControl requestCacheControl, final ResponseCacheControl responseCacheControl, @@ -364,14 +394,13 @@ ClassicHttpResponse revalidateCacheEntryWithFallback( try { response = revalidateCacheEntry(responseCacheControl, hit, target, request, scope, chain); } catch (final IOException ex) { + LOG.debug(ex.getMessage(), ex); + context.setAttribute(HttpCacheContext.CACHE_RESPONSE_STATUS, CacheResponseStatus.CACHE_MODULE_RESPONSE); if (suitabilityChecker.isSuitableIfError(requestCacheControl, responseCacheControl, hit.entry, getCurrentDate())) { - if (LOG.isDebugEnabled()) { - LOG.debug("Serving stale response due to IOException and stale-if-error enabled"); - } - return convert(unvalidatedCacheHit(request, context, hit.entry), scope); + LOG.debug("Serving stale response due to IOException and stale-if-error enabled"); + return convert(unvalidatedCacheHit(request, hit.entry)); } else { - LOG.debug(ex.getMessage(), ex); - return convert(generateGatewayTimeout(context), scope); + return convert(generateGatewayTimeout()); } } final int status = response.getCode(); @@ -381,7 +410,8 @@ ClassicHttpResponse revalidateCacheEntryWithFallback( LOG.debug("Serving stale response due to {} status and stale-if-error enabled", status); } EntityUtils.consume(response.getEntity()); - return convert(unvalidatedCacheHit(request, context, hit.entry), scope); + context.setAttribute(HttpCacheContext.CACHE_RESPONSE_STATUS, CacheResponseStatus.CACHE_MODULE_RESPONSE); + return convert(unvalidatedCacheHit(request, hit.entry)); } return response; } @@ -389,7 +419,6 @@ ClassicHttpResponse revalidateCacheEntryWithFallback( ClassicHttpResponse handleBackendResponse( final HttpHost target, final ClassicHttpRequest request, - final ExecChain.Scope scope, final Instant requestDate, final Instant responseDate, final ClassicHttpResponse backendResponse) throws IOException { @@ -403,7 +432,7 @@ ClassicHttpResponse handleBackendResponse( final boolean cacheable = responseCachingPolicy.isResponseCacheable(responseCacheControl, request, backendResponse); if (cacheable) { storeRequestIfModifiedSinceFor304Response(request, backendResponse); - return cacheAndReturnResponse(target, request, backendResponse, scope, requestDate, responseDate); + return cacheAndReturnResponse(target, request, backendResponse, requestDate, responseDate); } LOG.debug("Backend response is not cacheable"); return backendResponse; @@ -413,7 +442,6 @@ ClassicHttpResponse cacheAndReturnResponse( final HttpHost target, final HttpRequest request, final ClassicHttpResponse backendResponse, - final ExecChain.Scope scope, final Instant requestSent, final Instant responseReceived) throws IOException { LOG.debug("Caching backend response"); @@ -430,7 +458,7 @@ ClassicHttpResponse cacheAndReturnResponse( backendResponse, requestSent, responseReceived); - return convert(responseGenerator.generateResponse(request, updated.entry), scope); + return convert(responseGenerator.generateResponse(request, updated.entry)); } } @@ -470,7 +498,7 @@ ClassicHttpResponse cacheAndReturnResponse( hit = responseCache.store(target, request, backendResponse, buf, requestSent, responseReceived); LOG.debug("Backend response successfully cached (freshness check skipped)"); } - return convert(responseGenerator.generateResponse(request, hit.entry), scope); + return convert(responseGenerator.generateResponse(request, hit.entry)); } private ClassicHttpResponse handleCacheMiss( @@ -480,10 +508,15 @@ private ClassicHttpResponse handleCacheMiss( final ClassicHttpRequest request, final ExecChain.Scope scope, final ExecChain chain) throws IOException, HttpException { - recordCacheMiss(target, request); + cacheMisses.getAndIncrement(); + if (LOG.isDebugEnabled()) { + LOG.debug("Request {} {}: cache miss", request.getMethod(), request.getRequestUri()); + } + final HttpClientContext context = scope.clientContext; if (!mayCallBackend(requestCacheControl)) { - return new BasicClassicHttpResponse(HttpStatus.SC_GATEWAY_TIMEOUT, "Gateway Timeout"); + context.setAttribute(HttpCacheContext.CACHE_RESPONSE_STATUS, CacheResponseStatus.CACHE_MODULE_RESPONSE); + return convert(generateGatewayTimeout()); } if (partialMatch != null && partialMatch.entry.hasVariants() && request.getEntity() == null) { final List variants = responseCache.getVariants(partialMatch); @@ -517,7 +550,7 @@ ClassicHttpResponse negotiateResponseFromVariants( final Instant responseDate = getCurrentDate(); if (backendResponse.getCode() != HttpStatus.SC_NOT_MODIFIED) { - return handleBackendResponse(target, request, scope, requestDate, responseDate, backendResponse); + return handleBackendResponse(target, request, requestDate, responseDate, backendResponse); } else { // 304 response are not expected to have an enclosed content body, but still backendResponse.close(); @@ -541,13 +574,15 @@ ClassicHttpResponse negotiateResponseFromVariants( return callBackend(target, unconditional, scope, chain); } - recordCacheUpdate(scope.clientContext); + final HttpClientContext context = scope.clientContext; + context.setAttribute(HttpCacheContext.CACHE_RESPONSE_STATUS, CacheResponseStatus.VALIDATED); + cacheUpdates.getAndIncrement(); final CacheHit hit = responseCache.storeFromNegotiated(match, target, request, backendResponse, requestDate, responseDate); - if (shouldSendNotModifiedResponse(request, hit.entry, Instant.now())) { - return convert(responseGenerator.generateNotModifiedResponse(hit.entry), scope); + if (shouldSendNotModifiedResponse(request, hit.entry, responseDate)) { + return convert(responseGenerator.generateNotModifiedResponse(hit.entry)); } else { - return convert(responseGenerator.generateResponse(request, hit.entry), scope); + return convert(responseGenerator.generateResponse(request, hit.entry)); } } catch (final IOException | RuntimeException ex) { backendResponse.close(); diff --git a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachingExecBase.java b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachingExecBase.java index a26aec2e1b..4596e02238 100644 --- a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachingExecBase.java +++ b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachingExecBase.java @@ -31,19 +31,15 @@ import java.util.concurrent.atomic.AtomicLong; import org.apache.hc.client5.http.async.methods.SimpleHttpResponse; -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.ResourceIOException; import org.apache.hc.core5.http.EntityDetails; import org.apache.hc.core5.http.Header; import org.apache.hc.core5.http.HttpHeaders; -import org.apache.hc.core5.http.HttpHost; import org.apache.hc.core5.http.HttpRequest; import org.apache.hc.core5.http.HttpResponse; import org.apache.hc.core5.http.HttpStatus; import org.apache.hc.core5.http.Method; -import org.apache.hc.core5.http.protocol.HttpContext; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -117,57 +113,23 @@ public long getCacheUpdates() { return cacheUpdates.get(); } - void recordCacheMiss(final HttpHost target, final HttpRequest request) { - cacheMisses.getAndIncrement(); - if (LOG.isDebugEnabled()) { - LOG.debug("Cache miss [host: {}; uri: {}]", target, request.getRequestUri()); - } - } - - void recordCacheHit(final HttpHost target, final HttpRequest request) { - cacheHits.getAndIncrement(); - if (LOG.isDebugEnabled()) { - LOG.debug("Cache hit [host: {}; uri: {}]", target, request.getRequestUri()); - } - } - - void recordCacheFailure(final HttpHost target, final HttpRequest request) { - cacheMisses.getAndIncrement(); - if (LOG.isDebugEnabled()) { - LOG.debug("Cache failure [host: {}; uri: {}]", target, request.getRequestUri()); - } - } - - void recordCacheUpdate(final HttpContext context) { - cacheUpdates.getAndIncrement(); - setResponseStatus(context, CacheResponseStatus.VALIDATED); - } - SimpleHttpResponse generateCachedResponse( final HttpRequest request, - final HttpContext context, - final HttpCacheEntry entry) throws ResourceIOException { - setResponseStatus(context, CacheResponseStatus.CACHE_HIT); - if (shouldSendNotModifiedResponse(request, entry, Instant.now())) { + final HttpCacheEntry entry, + final Instant now) throws ResourceIOException { + if (shouldSendNotModifiedResponse(request, entry, now)) { return responseGenerator.generateNotModifiedResponse(entry); } else { return responseGenerator.generateResponse(request, entry); } } - SimpleHttpResponse generateGatewayTimeout( - final HttpContext context) { - setResponseStatus(context, CacheResponseStatus.CACHE_MODULE_RESPONSE); + SimpleHttpResponse generateGatewayTimeout() { return SimpleHttpResponse.create(HttpStatus.SC_GATEWAY_TIMEOUT, "Gateway Timeout"); } - SimpleHttpResponse unvalidatedCacheHit( - final HttpRequest request, - final HttpContext context, - final HttpCacheEntry entry) throws IOException { - final SimpleHttpResponse cachedResponse = responseGenerator.generateResponse(request, entry); - setResponseStatus(context, CacheResponseStatus.CACHE_HIT); - return cachedResponse; + SimpleHttpResponse unvalidatedCacheHit(final HttpRequest request, final HttpCacheEntry entry) throws IOException { + return responseGenerator.generateResponse(request, entry); } boolean mayCallBackend(final RequestCacheControl requestCacheControl) { @@ -178,12 +140,6 @@ boolean mayCallBackend(final RequestCacheControl requestCacheControl) { return true; } - void setResponseStatus(final HttpContext context, final CacheResponseStatus value) { - if (context != null) { - context.setAttribute(HttpCacheContext.CACHE_RESPONSE_STATUS, value); - } - } - Instant getCurrentDate() { return Instant.now(); } 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 c7b120f15f..961e6615eb 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 @@ -900,8 +900,7 @@ public void testTooLargeResponsesAreNotCached() throws Exception { originResponse.setHeader("Date", DateUtils.formatStandardDate(responseGenerated)); originResponse.setHeader("ETag", "\"etag\""); - final ExecChain.Scope scope = new ExecChain.Scope("test", route, request, mockExecRuntime, context); - impl.cacheAndReturnResponse(host, request, originResponse, scope, requestSent, responseReceived); + impl.cacheAndReturnResponse(host, request, originResponse, requestSent, responseReceived); Mockito.verify(cache, Mockito.never()).store( Mockito.any(), Mockito.any(), Mockito.any(), Mockito.any(), Mockito.any(), Mockito.any()); @@ -937,8 +936,7 @@ public void testSmallEnoughResponsesAreCached() throws Exception { Mockito.eq(requestSent), Mockito.eq(responseReceived))).thenReturn(new CacheHit("key", httpCacheEntry)); - final ExecChain.Scope scope = new ExecChain.Scope("test", route, request, mockExecRuntime, context); - impl.cacheAndReturnResponse(host, request, originResponse, scope, requestSent, responseReceived); + impl.cacheAndReturnResponse(host, request, originResponse, requestSent, responseReceived); Mockito.verify(mockCache).store( Mockito.any(), @@ -958,18 +956,6 @@ public void testIfOnlyIfCachedAndNoCacheEntryBackendNotCalled() throws Exception Assertions.assertEquals(HttpStatus.SC_GATEWAY_TIMEOUT, resp.getCode()); } - @Test - public void testSetsRouteInContextOnCacheHit() throws Exception { - final ClassicHttpResponse response = HttpTestUtils.make200Response(); - response.setHeader("Cache-Control", "max-age=3600"); - Mockito.when(mockExecChain.proceed(RequestEquivalent.eq(request), Mockito.any())).thenReturn(response); - - final HttpClientContext ctx = HttpClientContext.create(); - impl.execute(request, new ExecChain.Scope("test", route, request, mockExecRuntime, context), mockExecChain); - impl.execute(request, new ExecChain.Scope("test", route, request, mockExecRuntime, ctx), mockExecChain); - Assertions.assertEquals(route, ctx.getHttpRoute()); - } - @Test public void testSetsRequestInContextOnCacheHit() throws Exception { final ClassicHttpResponse response = HttpTestUtils.make200Response(); @@ -1288,7 +1274,7 @@ public void testNotModifiedResponseUpdatesCacheEntry() throws Exception { .thenReturn(new CacheHit("key", cacheEntry)); // Call cacheAndReturnResponse with 304 Not Modified response - final ClassicHttpResponse cachedResponse = impl.cacheAndReturnResponse(host, request, backendResponse, scope, requestSent, responseReceived); + final ClassicHttpResponse cachedResponse = impl.cacheAndReturnResponse(host, request, backendResponse, requestSent, responseReceived); // Verify cache entry is updated Mockito.verify(mockCache).update( From 54b63c52d6bb17b0d46ca291ac1dfe10799ea66b Mon Sep 17 00:00:00 2001 From: Oleg Kalnichevski Date: Wed, 15 Nov 2023 13:37:26 +0100 Subject: [PATCH 7/8] Better debug logging in the caching protocol handlers --- .../http/impl/cache/AsyncCachingExec.java | 177 +++++++++++----- .../impl/cache/CacheControlHeaderParser.java | 2 +- .../impl/cache/CacheableRequestPolicy.java | 12 +- .../CachedResponseSuitabilityChecker.java | 9 + .../client5/http/impl/cache/CachingExec.java | 195 +++++++++++------- .../http/impl/cache/CachingExecBase.java | 12 -- .../http/impl/cache/RequestCacheControl.java | 41 +++- .../http/impl/cache/ResponseCacheControl.java | 58 ++++-- .../cache/TestCacheableRequestPolicy.java | 18 +- .../http/impl/cache/TestCachingExecChain.java | 6 +- 10 files changed, 344 insertions(+), 186 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 570136bb7c..0d7261a017 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 @@ -46,10 +46,8 @@ import org.apache.hc.client5.http.async.methods.SimpleBody; import org.apache.hc.client5.http.async.methods.SimpleHttpResponse; import org.apache.hc.client5.http.cache.CacheResponseStatus; -import org.apache.hc.client5.http.cache.HttpAsyncCacheStorage; import org.apache.hc.client5.http.cache.HttpCacheContext; import org.apache.hc.client5.http.cache.HttpCacheEntry; -import org.apache.hc.client5.http.cache.ResourceFactory; import org.apache.hc.client5.http.cache.ResourceIOException; import org.apache.hc.client5.http.impl.ExecSupport; import org.apache.hc.client5.http.protocol.HttpClientContext; @@ -69,6 +67,7 @@ import org.apache.hc.core5.http.HttpResponse; import org.apache.hc.core5.http.HttpStatus; import org.apache.hc.core5.http.impl.BasicEntityDetails; +import org.apache.hc.core5.http.message.RequestLine; import org.apache.hc.core5.http.nio.AsyncDataConsumer; import org.apache.hc.core5.http.nio.AsyncEntityProducer; import org.apache.hc.core5.http.nio.CapacityChannel; @@ -117,15 +116,6 @@ class AsyncCachingExec extends CachingExecBase implements AsyncExecChainHandler config); } - AsyncCachingExec( - final ResourceFactory resourceFactory, - final HttpAsyncCacheStorage storage, - final ScheduledExecutorService executorService, - final SchedulingStrategy schedulingStrategy, - final CacheConfig config) { - this(new BasicHttpAsyncCache(resourceFactory, storage), executorService, schedulingStrategy, config); - } - private void triggerResponse( final SimpleHttpResponse cacheResponse, final AsyncExecChain.Scope scope, @@ -245,9 +235,14 @@ public void doExecute( final AsyncExecChain chain, final AsyncExecCallback asyncExecCallback) throws HttpException, IOException { + final String exchangeId = scope.exchangeId; final HttpClientContext context = scope.clientContext; final CancellableDependency operation = scope.cancellableDependency; + if (LOG.isDebugEnabled()) { + LOG.debug("{} request via cache: {}", exchangeId, new RequestLine(request)); + } + context.setAttribute(HttpCacheContext.CACHE_RESPONSE_STATUS, CacheResponseStatus.CACHE_MISS); if (clientRequestsOurOptions(request)) { @@ -258,10 +253,10 @@ public void doExecute( final RequestCacheControl requestCacheControl = CacheControlHeaderParser.INSTANCE.parse(request); if (LOG.isDebugEnabled()) { - LOG.debug("Request cache control: {}", requestCacheControl); + LOG.debug("{} request cache control: {}", exchangeId, requestCacheControl); } - if (cacheableRequestPolicy.isServableFromCache(requestCacheControl, request)) { + if (cacheableRequestPolicy.canBeServedFromCache(requestCacheControl, request)) { operation.setDependency(responseCache.match(target, request, new FutureCallback() { @Override @@ -269,12 +264,11 @@ public void completed(final CacheMatch result) { final CacheHit hit = result != null ? result.hit : null; final CacheHit root = result != null ? result.root : null; if (hit == null) { - LOG.debug("Cache miss"); handleCacheMiss(requestCacheControl, root, target, request, entityProducer, scope, chain, asyncExecCallback); } else { final ResponseCacheControl responseCacheControl = CacheControlHeaderParser.INSTANCE.parse(hit.entry); if (LOG.isDebugEnabled()) { - LOG.debug("Response cache control: {}", responseCacheControl); + LOG.debug("{} response cache control: {}", exchangeId, responseCacheControl); } handleCacheHit(requestCacheControl, responseCacheControl, hit, target, request, entityProducer, scope, chain, asyncExecCallback); } @@ -293,7 +287,9 @@ public void cancelled() { })); } else { - LOG.debug("Request is not servable from cache"); + if (LOG.isDebugEnabled()) { + LOG.debug("{} request cannot be served from cache", exchangeId); + } callBackend(target, request, entityProducer, scope, chain, asyncExecCallback); } } @@ -318,7 +314,11 @@ void callBackend( final AsyncExecChain.Scope scope, final AsyncExecChain chain, final AsyncExecCallback asyncExecCallback) { - LOG.debug("Calling the backend"); + final String exchangeId = scope.exchangeId; + + if (LOG.isDebugEnabled()) { + LOG.debug("{} calling the backend", exchangeId); + } final Instant requestDate = getCurrentDate(); final AtomicReference callbackRef = new AtomicReference<>(); chainProceed(request, entityProducer, scope, chain, new AsyncExecCallback() { @@ -368,6 +368,7 @@ public void failed(final Exception cause) { class CachingAsyncDataConsumer implements AsyncDataConsumer { + private final String exchangeId; private final AsyncExecCallback fallback; private final HttpResponse backendResponse; private final EntityDetails entityDetails; @@ -376,9 +377,11 @@ class CachingAsyncDataConsumer implements AsyncDataConsumer { private final AtomicReference dataConsumerRef; CachingAsyncDataConsumer( + final String exchangeId, final AsyncExecCallback fallback, final HttpResponse backendResponse, final EntityDetails entityDetails) { + this.exchangeId = exchangeId; this.fallback = fallback; this.backendResponse = backendResponse; this.entityDetails = entityDetails; @@ -409,7 +412,9 @@ public final void consume(final ByteBuffer src) throws IOException { } } if (buffer.length() > cacheConfig.getMaxObjectSize()) { - LOG.debug("Backend response content length exceeds maximum"); + if (LOG.isDebugEnabled()) { + LOG.debug("{} backend response content length exceeds maximum", exchangeId); + } // Over the max limit. Stop buffering and forward the response // along with all the data buffered so far to the caller. bufferRef.set(null); @@ -480,6 +485,7 @@ class BackendResponseHandler implements AsyncExecCallback { public AsyncDataConsumer handleResponse( final HttpResponse backendResponse, final EntityDetails entityDetails) throws HttpException, IOException { + final String exchangeId = scope.exchangeId; responseCache.evictInvalidatedEntries(target, request, backendResponse, new FutureCallback() { @Override @@ -488,7 +494,9 @@ public void completed(final Boolean result) { @Override public void failed(final Exception ex) { - LOG.warn("Unable to flush invalidated entries from cache", ex); + if (LOG.isDebugEnabled()) { + LOG.debug("{} unable to flush invalidated entries from cache", exchangeId, ex); + } } @Override @@ -497,21 +505,27 @@ public void cancelled() { }); if (isResponseTooBig(entityDetails)) { - LOG.debug("Backend response is known to be too big"); + if (LOG.isDebugEnabled()) { + LOG.debug("{} backend response is known to be too big", exchangeId); + } return asyncExecCallback.handleResponse(backendResponse, entityDetails); } final ResponseCacheControl responseCacheControl = CacheControlHeaderParser.INSTANCE.parse(backendResponse); final boolean cacheable = responseCachingPolicy.isResponseCacheable(responseCacheControl, request, backendResponse); if (cacheable) { - cachingConsumerRef.set(new CachingAsyncDataConsumer(asyncExecCallback, backendResponse, entityDetails)); + cachingConsumerRef.set(new CachingAsyncDataConsumer(exchangeId, asyncExecCallback, backendResponse, entityDetails)); storeRequestIfModifiedSinceFor304Response(request, backendResponse); } else { - LOG.debug("Backend response is not cacheable"); + if (LOG.isDebugEnabled()) { + LOG.debug("{} backend response is not cacheable", exchangeId); + } } final CachingAsyncDataConsumer cachingDataConsumer = cachingConsumerRef.get(); if (cachingDataConsumer != null) { - LOG.debug("Caching backend response"); + if (LOG.isDebugEnabled()) { + LOG.debug("{} caching backend response", exchangeId); + } return cachingDataConsumer; } return asyncExecCallback.handleResponse(backendResponse, entityDetails); @@ -523,6 +537,7 @@ public void handleInformationResponse(final HttpResponse response) throws HttpEx } void triggerNewCacheEntryResponse(final HttpResponse backendResponse, final Instant responseDate, final ByteArrayBuffer buffer) { + final String exchangeId = scope.exchangeId; final CancellableDependency operation = scope.cancellableDependency; operation.setDependency(responseCache.store( target, @@ -535,7 +550,9 @@ void triggerNewCacheEntryResponse(final HttpResponse backendResponse, final Inst @Override public void completed(final CacheHit hit) { - LOG.debug("Backend response successfully cached"); + if (LOG.isDebugEnabled()) { + LOG.debug("{} backend response successfully cached", exchangeId); + } try { final SimpleHttpResponse cacheResponse = responseGenerator.generateResponse(request, hit.entry); triggerResponse(cacheResponse, scope, asyncExecCallback); @@ -560,6 +577,7 @@ public void cancelled() { @Override public void completed() { + final String exchangeId = scope.exchangeId; final CachingAsyncDataConsumer cachingDataConsumer = cachingConsumerRef.getAndSet(null); if (cachingDataConsumer != null && !cachingDataConsumer.writtenThrough.get()) { final ByteArrayBuffer buffer = cachingDataConsumer.bufferRef.getAndSet(null); @@ -572,7 +590,9 @@ public void completed() { public void completed(final CacheMatch result) { final CacheHit hit = result != null ? result.hit : null; if (HttpCacheEntry.isNewer(hit != null ? hit.entry : null, backendResponse)) { - LOG.debug("Backend already contains fresher cache entry"); + if (LOG.isDebugEnabled()) { + LOG.debug("{} backend already contains fresher cache entry", exchangeId); + } try { final SimpleHttpResponse cacheResponse = responseGenerator.generateResponse(request, hit.entry); triggerResponse(cacheResponse, scope, asyncExecCallback); @@ -621,9 +641,12 @@ private void handleCacheHit( final AsyncExecChain chain, final AsyncExecCallback asyncExecCallback) { final HttpClientContext context = scope.clientContext; + final String exchangeId = scope.exchangeId; + if (LOG.isDebugEnabled()) { - LOG.debug("Request {} {}: cache hit", request.getMethod(), request.getRequestUri()); + LOG.debug("{} cache hit: {}", exchangeId, new RequestLine(request)); } + context.setAttribute(HttpCacheContext.CACHE_RESPONSE_STATUS, CacheResponseStatus.CACHE_HIT); cacheHits.getAndIncrement(); @@ -631,15 +654,20 @@ private void handleCacheHit( final CacheSuitability cacheSuitability = suitabilityChecker.assessSuitability(requestCacheControl, responseCacheControl, request, hit.entry, now); if (LOG.isDebugEnabled()) { - LOG.debug("Request {} {}: {}", request.getMethod(), request.getRequestUri(), cacheSuitability); + LOG.debug("{} cache suitability: {}", exchangeId, cacheSuitability); } if (cacheSuitability == CacheSuitability.FRESH || cacheSuitability == CacheSuitability.FRESH_ENOUGH) { - LOG.debug("Cache hit is suitable"); + if (LOG.isDebugEnabled()) { + LOG.debug("{} cache hit is fresh enough", exchangeId); + } try { final SimpleHttpResponse cacheResponse = generateCachedResponse(request, hit.entry, now); triggerResponse(cacheResponse, scope, asyncExecCallback); } catch (final ResourceIOException ex) { - if (!mayCallBackend(requestCacheControl)) { + if (requestCacheControl.isOnlyIfCached()) { + if (LOG.isDebugEnabled()) { + LOG.debug("{} request marked only-if-cached", exchangeId); + } context.setAttribute(HttpCacheContext.CACHE_RESPONSE_STATUS, CacheResponseStatus.CACHE_MODULE_RESPONSE); final SimpleHttpResponse cacheResponse = generateGatewayTimeout(); triggerResponse(cacheResponse, scope, asyncExecCallback); @@ -653,31 +681,43 @@ private void handleCacheHit( } } } else { - if (!mayCallBackend(requestCacheControl)) { - LOG.debug("Cache entry not is not fresh and only-if-cached requested"); + if (requestCacheControl.isOnlyIfCached()) { + if (LOG.isDebugEnabled()) { + LOG.debug("{} cache entry not is not fresh and only-if-cached requested", exchangeId); + } context.setAttribute(HttpCacheContext.CACHE_RESPONSE_STATUS, CacheResponseStatus.CACHE_MODULE_RESPONSE); final SimpleHttpResponse cacheResponse = generateGatewayTimeout(); triggerResponse(cacheResponse, scope, asyncExecCallback); } else if (cacheSuitability == CacheSuitability.MISMATCH) { - LOG.debug("Cache entry does not match the request; calling backend"); + if (LOG.isDebugEnabled()) { + LOG.debug("{} cache entry does not match the request; calling backend", exchangeId); + } callBackend(target, request, entityProducer, scope, chain, asyncExecCallback); } else if (entityProducer != null && !entityProducer.isRepeatable()) { - LOG.debug("Request is not repeatable; calling backend"); + if (LOG.isDebugEnabled()) { + LOG.debug("{} request is not repeatable; calling backend", exchangeId); + } callBackend(target, request, entityProducer, scope, chain, asyncExecCallback); } else if (hit.entry.getStatus() == HttpStatus.SC_NOT_MODIFIED && !suitabilityChecker.isConditional(request)) { - LOG.debug("Non-modified cache entry does not match the non-conditional request; calling backend"); + if (LOG.isDebugEnabled()) { + LOG.debug("{} non-modified cache entry does not match the non-conditional request; calling backend", exchangeId); + } callBackend(target, request, entityProducer, scope, chain, asyncExecCallback); } else if (cacheSuitability == CacheSuitability.REVALIDATION_REQUIRED) { - LOG.debug("Revalidation required; revalidating cache entry"); + if (LOG.isDebugEnabled()) { + LOG.debug("{} revalidation required; revalidating cache entry", exchangeId); + } revalidateCacheEntryWithoutFallback(responseCacheControl, hit, target, request, entityProducer, scope, chain, asyncExecCallback); } else if (cacheSuitability == CacheSuitability.STALE_WHILE_REVALIDATED) { if (cacheRevalidator != null) { - LOG.debug("Serving stale with asynchronous revalidation"); + if (LOG.isDebugEnabled()) { + LOG.debug("{} serving stale with asynchronous revalidation", exchangeId); + } try { - final String exchangeId = ExecSupport.getNextExchangeId(); - context.setExchangeId(exchangeId); + final String revalidationExchangeId = ExecSupport.getNextExchangeId(); + context.setExchangeId(revalidationExchangeId); final AsyncExecChain.Scope fork = new AsyncExecChain.Scope( - exchangeId, + revalidationExchangeId, scope.route, scope.originalRequest, new ComplexFuture<>(null), @@ -685,6 +725,9 @@ private void handleCacheHit( scope.execRuntime.fork(), scope.scheduler, scope.execCount); + if (LOG.isDebugEnabled()) { + LOG.debug("{} starting asynchronous revalidation exchange {}", exchangeId, revalidationExchangeId); + } cacheRevalidator.revalidateCacheEntry( hit.getEntryKey(), asyncExecCallback, @@ -696,14 +739,20 @@ private void handleCacheHit( asyncExecCallback.failed(ex); } } else { - LOG.debug("Revalidating stale cache entry (asynchronous revalidation disabled)"); + if (LOG.isDebugEnabled()) { + LOG.debug("{} revalidating stale cache entry (asynchronous revalidation disabled)", exchangeId); + } revalidateCacheEntryWithFallback(requestCacheControl, responseCacheControl, hit, target, request, entityProducer, scope, chain, asyncExecCallback); } } else if (cacheSuitability == CacheSuitability.STALE) { - LOG.debug("Revalidating stale cache entry"); + if (LOG.isDebugEnabled()) { + LOG.debug("{} revalidating stale cache entry", exchangeId); + } revalidateCacheEntryWithFallback(requestCacheControl, responseCacheControl, hit, target, request, entityProducer, scope, chain, asyncExecCallback); } else { - LOG.debug("Cache entry not usable; calling backend"); + if (LOG.isDebugEnabled()) { + LOG.debug("{} cache entry not usable; calling backend", exchangeId); + } callBackend(target, request, entityProducer, scope, chain, asyncExecCallback); } } @@ -880,6 +929,7 @@ void revalidateCacheEntryWithoutFallback( final AsyncExecChain.Scope scope, final AsyncExecChain chain, final AsyncExecCallback asyncExecCallback) { + final String exchangeId = scope.exchangeId; final HttpClientContext context = scope.clientContext; revalidateCacheEntry(responseCacheControl, hit, target, request, entityProducer, scope, chain, new AsyncExecCallback() { @@ -905,7 +955,9 @@ public void completed() { @Override public void failed(final Exception cause) { if (!committed.get() && cause instanceof IOException) { - LOG.debug(cause.getMessage(), cause); + if (LOG.isDebugEnabled()) { + LOG.debug("{} I/O error while revalidating cache entry", exchangeId, cause); + } final SimpleHttpResponse cacheResponse = generateGatewayTimeout(); context.setAttribute(HttpCacheContext.CACHE_RESPONSE_STATUS, CacheResponseStatus.CACHE_MODULE_RESPONSE); triggerResponse(cacheResponse, scope, asyncExecCallback); @@ -927,6 +979,7 @@ void revalidateCacheEntryWithFallback( final AsyncExecChain.Scope scope, final AsyncExecChain chain, final AsyncExecCallback asyncExecCallback) { + final String exchangeId = scope.exchangeId; final HttpClientContext context = scope.clientContext; revalidateCacheEntry(responseCacheControl, hit, target, request, entityProducer, scope, chain, new AsyncExecCallback() { @@ -938,7 +991,7 @@ public AsyncDataConsumer handleResponse(final HttpResponse response, final Entit if (staleIfErrorAppliesTo(status) && suitabilityChecker.isSuitableIfError(requestCacheControl, responseCacheControl, hit.entry, getCurrentDate())) { if (LOG.isDebugEnabled()) { - LOG.debug("Serving stale response due to {} status and stale-if-error enabled", status); + LOG.debug("{} serving stale response due to {} status and stale-if-error enabled", exchangeId, status); } return null; } else { @@ -972,11 +1025,15 @@ public void completed() { public void failed(final Exception cause) { final HttpResponse response = committed.get(); if (response == null) { - LOG.debug(cause.getMessage(), cause); + if (LOG.isDebugEnabled()) { + LOG.debug("{} I/O error while revalidating cache entry", exchangeId, cause); + } context.setAttribute(HttpCacheContext.CACHE_RESPONSE_STATUS, CacheResponseStatus.CACHE_MODULE_RESPONSE); if (cause instanceof IOException && suitabilityChecker.isSuitableIfError(requestCacheControl, responseCacheControl, hit.entry, getCurrentDate())) { - LOG.debug("Serving stale response due to IOException and stale-if-error enabled"); + if (LOG.isDebugEnabled()) { + LOG.debug("{} serving stale response due to IOException and stale-if-error enabled", exchangeId); + } try { final SimpleHttpResponse cacheResponse = unvalidatedCacheHit(request, hit.entry); triggerResponse(cacheResponse, scope, asyncExecCallback); @@ -1003,13 +1060,18 @@ private void handleCacheMiss( final AsyncExecChain.Scope scope, final AsyncExecChain chain, final AsyncExecCallback asyncExecCallback) { - cacheMisses.getAndIncrement(); + final String exchangeId = scope.exchangeId; + if (LOG.isDebugEnabled()) { - LOG.debug("Request {} {}: cache miss", request.getMethod(), request.getRequestUri()); + LOG.debug("{} cache miss: {}", exchangeId, new RequestLine(request)); } + cacheMisses.getAndIncrement(); final CancellableDependency operation = scope.cancellableDependency; - if (!mayCallBackend(requestCacheControl)) { + if (requestCacheControl.isOnlyIfCached()) { + if (LOG.isDebugEnabled()) { + LOG.debug("{} request marked only-if-cached", exchangeId); + } final HttpClientContext context = scope.clientContext; context.setAttribute(HttpCacheContext.CACHE_RESPONSE_STATUS, CacheResponseStatus.CACHE_MODULE_RESPONSE); final SimpleHttpResponse cacheResponse = generateGatewayTimeout(); @@ -1054,6 +1116,7 @@ void negotiateResponseFromVariants( final AsyncExecChain chain, final AsyncExecCallback asyncExecCallback, final Collection variants) { + final String exchangeId = scope.exchangeId; final CancellableDependency operation = scope.cancellableDependency; final Map variantMap = new HashMap<>(); for (final CacheHit variant : variants) { @@ -1127,7 +1190,7 @@ public void completed(final CacheMatch result) { final CacheHit hit = result != null ? result.hit : null; if (hit != null) { if (LOG.isDebugEnabled()) { - LOG.debug("Existing cache entry found, updating cache entry"); + LOG.debug("{} existing cache entry found, updating cache entry", exchangeId); } responseCache.update( hit, @@ -1142,7 +1205,7 @@ public void completed(final CacheMatch result) { public void completed(final CacheHit updated) { try { if (LOG.isDebugEnabled()) { - LOG.debug("Cache entry updated, generating response from updated entry"); + LOG.debug("{} cache entry updated, generating response from updated entry", exchangeId); } final SimpleHttpResponse cacheResponse = responseGenerator.generateResponse(request, updated.entry); triggerResponse(cacheResponse, scope, asyncExecCallback); @@ -1153,7 +1216,7 @@ public void completed(final CacheHit updated) { @Override public void failed(final Exception cause) { if (LOG.isDebugEnabled()) { - LOG.debug("Request failed: {}", cause.getMessage()); + LOG.debug("{} request failed: {}", exchangeId, cause.getMessage()); } asyncExecCallback.failed(cause); } @@ -1161,7 +1224,7 @@ public void failed(final Exception cause) { @Override public void cancelled() { if (LOG.isDebugEnabled()) { - LOG.debug("Cache entry updated aborted"); + LOG.debug("{} cache entry updated aborted", exchangeId); } asyncExecCallback.failed(new InterruptedIOException()); } @@ -1187,13 +1250,17 @@ public void cancelled() { } else { final Header resultEtagHeader = backendResponse.getFirstHeader(HttpHeaders.ETAG); if (resultEtagHeader == null) { - LOG.warn("304 response did not contain ETag"); + if (LOG.isDebugEnabled()) { + LOG.debug("{} 304 response did not contain ETag", exchangeId); + } callback = new AsyncExecCallbackWrapper(() -> callBackend(target, request, entityProducer, scope, chain, asyncExecCallback), asyncExecCallback::failed); } else { final String resultEtag = resultEtagHeader.getValue(); final CacheHit match = variantMap.get(resultEtag); if (match == null) { - LOG.debug("304 response did not contain ETag matching one sent in If-None-Match"); + if (LOG.isDebugEnabled()) { + LOG.debug("{} 304 response did not contain ETag matching one sent in If-None-Match", exchangeId); + } callback = new AsyncExecCallbackWrapper(() -> callBackend(target, request, entityProducer, scope, chain, asyncExecCallback), asyncExecCallback::failed); } else { if (HttpCacheEntry.isNewer(match.entry, backendResponse)) { diff --git a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CacheControlHeaderParser.java b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CacheControlHeaderParser.java index a03e9310bd..f6c0d773b7 100644 --- a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CacheControlHeaderParser.java +++ b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CacheControlHeaderParser.java @@ -252,7 +252,7 @@ public final RequestCacheControl parse(final HttpRequest request) { private static long parseSeconds(final String name, final String value) { final long delta = CacheSupport.deltaSeconds(value); if (delta == -1 && LOG.isDebugEnabled()) { - LOG.debug("Directive {} was malformed: {}", name, value); + LOG.debug("Directive {} is malformed: {}", name, value); } return delta; } diff --git a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CacheableRequestPolicy.java b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CacheableRequestPolicy.java index 79495feb2a..7ea900f491 100644 --- a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CacheableRequestPolicy.java +++ b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CacheableRequestPolicy.java @@ -47,31 +47,31 @@ class CacheableRequestPolicy { * an HttpRequest * @return boolean Is it possible to serve this request from cache */ - public boolean isServableFromCache(final RequestCacheControl cacheControl, final HttpRequest request) { + public boolean canBeServedFromCache(final RequestCacheControl cacheControl, final HttpRequest request) { final String method = request.getMethod(); final ProtocolVersion pv = request.getVersion() != null ? request.getVersion() : HttpVersion.DEFAULT; if (HttpVersion.HTTP_1_1.compareToVersion(pv) != 0) { - LOG.debug("non-HTTP/1.1 request is not serveable from cache"); + LOG.debug("non-HTTP/1.1 request cannot be served from cache"); return false; } if (!Method.GET.isSame(method) && !Method.HEAD.isSame(method)) { if (LOG.isDebugEnabled()) { - LOG.debug("{} request is not serveable from cache", method); + LOG.debug("{} request cannot be served from cache", method); } return false; } if (cacheControl.isNoStore()) { - LOG.debug("Request with no-store is not serveable from cache"); + LOG.debug("Request with no-store cannot be served from cache"); return false; } if (cacheControl.isNoCache()) { - LOG.debug("Request with no-cache is not serveable from cache"); + LOG.debug("Request with no-cache cannot be served from cache"); return false; } - LOG.debug("Request is serveable from cache"); + LOG.debug("Request can be served from cache"); return true; } diff --git a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachedResponseSuitabilityChecker.java b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachedResponseSuitabilityChecker.java index a81a59716d..4e354cafc9 100644 --- a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachedResponseSuitabilityChecker.java +++ b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachedResponseSuitabilityChecker.java @@ -131,7 +131,13 @@ public CacheSuitability assessSuitability(final RequestCacheControl requestCache } final TimeValue currentAge = validityStrategy.getCurrentAge(entry, now); + if (LOG.isDebugEnabled()) { + LOG.debug("Cache entry current age: {}", currentAge); + } final TimeValue freshnessLifetime = validityStrategy.getFreshnessLifetime(responseCacheControl, entry); + if (LOG.isDebugEnabled()) { + LOG.debug("Cache entry freshness lifetime: {}", freshnessLifetime); + } final boolean fresh = currentAge.compareTo(freshnessLifetime) < 0; @@ -162,6 +168,9 @@ public CacheSuitability assessSuitability(final RequestCacheControl requestCache if (requestCacheControl.getMaxStale() >= 0) { final long stale = currentAge.compareTo(freshnessLifetime) > 0 ? currentAge.toSeconds() - freshnessLifetime.toSeconds() : 0; + if (LOG.isDebugEnabled()) { + LOG.debug("Cache entry staleness: {} SECONDS", stale); + } if (stale >= requestCacheControl.getMaxStale()) { LOG.debug("Response from cache is not suitable due to the request max-stale requirement"); return CacheSuitability.REVALIDATION_REQUIRED; 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 6b21bb5011..60e9f99af7 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 @@ -33,7 +33,6 @@ import java.util.Iterator; import java.util.List; import java.util.Map; -import java.util.concurrent.ScheduledExecutorService; import org.apache.hc.client5.http.HttpRoute; import org.apache.hc.client5.http.async.methods.SimpleBody; @@ -47,7 +46,6 @@ import org.apache.hc.client5.http.classic.ExecChainHandler; import org.apache.hc.client5.http.impl.ExecSupport; import org.apache.hc.client5.http.protocol.HttpClientContext; -import org.apache.hc.client5.http.schedule.SchedulingStrategy; import org.apache.hc.core5.http.ClassicHttpRequest; import org.apache.hc.core5.http.ClassicHttpResponse; import org.apache.hc.core5.http.ContentType; @@ -64,6 +62,7 @@ import org.apache.hc.core5.http.io.entity.StringEntity; import org.apache.hc.core5.http.io.support.ClassicRequestBuilder; import org.apache.hc.core5.http.message.BasicClassicHttpResponse; +import org.apache.hc.core5.http.message.RequestLine; import org.apache.hc.core5.http.protocol.HttpCoreContext; import org.apache.hc.core5.net.URIAuthority; import org.apache.hc.core5.util.Args; @@ -115,32 +114,6 @@ class CachingExec extends CachingExecBase implements ExecChainHandler { ClassicRequestBuilder.copy(classicHttpRequest).build()); } - CachingExec( - final HttpCache responseCache, - final CacheValidityPolicy validityPolicy, - final ResponseCachingPolicy responseCachingPolicy, - final CachedHttpResponseGenerator responseGenerator, - final CacheableRequestPolicy cacheableRequestPolicy, - final CachedResponseSuitabilityChecker suitabilityChecker, - final DefaultCacheRevalidator cacheRevalidator, - final ConditionalRequestBuilder conditionalRequestBuilder, - final CacheConfig config) { - super(validityPolicy, responseCachingPolicy, responseGenerator, cacheableRequestPolicy, suitabilityChecker, config); - this.responseCache = responseCache; - this.cacheRevalidator = cacheRevalidator; - this.conditionalRequestBuilder = conditionalRequestBuilder; - } - - CachingExec( - final HttpCache cache, - final ScheduledExecutorService executorService, - final SchedulingStrategy schedulingStrategy, - final CacheConfig config) { - this(cache, - executorService != null ? new DefaultCacheRevalidator(executorService, schedulingStrategy) : null, - config); - } - @Override public ClassicHttpResponse execute( final ClassicHttpRequest request, @@ -168,35 +141,40 @@ ClassicHttpResponse doExecute( final ClassicHttpRequest request, final ExecChain.Scope scope, final ExecChain chain) throws IOException, HttpException { - + final String exchangeId = scope.exchangeId; final HttpClientContext context = scope.clientContext; + if (LOG.isDebugEnabled()) { + LOG.debug("{} request via cache: {}", exchangeId, new RequestLine(request)); + } + context.setAttribute(HttpCacheContext.CACHE_RESPONSE_STATUS, CacheResponseStatus.CACHE_MISS); if (clientRequestsOurOptions(request)) { context.setAttribute(HttpCacheContext.CACHE_RESPONSE_STATUS, CacheResponseStatus.CACHE_MODULE_RESPONSE); return new BasicClassicHttpResponse(HttpStatus.SC_NOT_IMPLEMENTED); } - final CacheMatch result = responseCache.match(target, request); - final CacheHit hit = result != null ? result.hit : null; - final CacheHit root = result != null ? result.root : null; - final RequestCacheControl requestCacheControl = CacheControlHeaderParser.INSTANCE.parse(request); if (LOG.isDebugEnabled()) { LOG.debug("Request cache control: {}", requestCacheControl); } - if (!cacheableRequestPolicy.isServableFromCache(requestCacheControl, request)) { - LOG.debug("Request is not servable from cache"); + if (!cacheableRequestPolicy.canBeServedFromCache(requestCacheControl, request)) { + if (LOG.isDebugEnabled()) { + LOG.debug("{} request cannot be served from cache", exchangeId); + } return callBackend(target, request, scope, chain); } + final CacheMatch result = responseCache.match(target, request); + final CacheHit hit = result != null ? result.hit : null; + final CacheHit root = result != null ? result.root : null; + if (hit == null) { - LOG.debug("Cache miss"); return handleCacheMiss(requestCacheControl, root, target, request, scope, chain); } else { final ResponseCacheControl responseCacheControl = CacheControlHeaderParser.INSTANCE.parse(hit.entry); if (LOG.isDebugEnabled()) { - LOG.debug("Response cache control: {}", responseCacheControl); + LOG.debug("{} response cache control: {}", exchangeId, responseCacheControl); } return handleCacheHit(requestCacheControl, responseCacheControl, hit, target, request, scope, chain); } @@ -231,12 +209,15 @@ ClassicHttpResponse callBackend( final ExecChain.Scope scope, final ExecChain chain) throws IOException, HttpException { + final String exchangeId = scope.exchangeId; final Instant requestDate = getCurrentDate(); - LOG.debug("Calling the backend"); + if (LOG.isDebugEnabled()) { + LOG.debug("{} calling the backend", exchangeId); + } final ClassicHttpResponse backendResponse = chain.proceed(request, scope); try { - return handleBackendResponse(target, request, requestDate, getCurrentDate(), backendResponse); + return handleBackendResponse(exchangeId, target, request, requestDate, getCurrentDate(), backendResponse); } catch (final IOException | RuntimeException ex) { backendResponse.close(); throw ex; @@ -251,10 +232,13 @@ private ClassicHttpResponse handleCacheHit( final ClassicHttpRequest request, final ExecChain.Scope scope, final ExecChain chain) throws IOException, HttpException { + final String exchangeId = scope.exchangeId; final HttpClientContext context = scope.clientContext; + if (LOG.isDebugEnabled()) { - LOG.debug("Request {} {}: cache hit", request.getMethod(), request.getRequestUri()); + LOG.debug("{} cache hit: {}", exchangeId, new RequestLine(request)); } + context.setAttribute(HttpCacheContext.CACHE_RESPONSE_STATUS, CacheResponseStatus.CACHE_HIT); cacheHits.getAndIncrement(); @@ -262,14 +246,19 @@ private ClassicHttpResponse handleCacheHit( final CacheSuitability cacheSuitability = suitabilityChecker.assessSuitability(requestCacheControl, responseCacheControl, request, hit.entry, now); if (LOG.isDebugEnabled()) { - LOG.debug("Request {} {}: {}", request.getMethod(), request.getRequestUri(), cacheSuitability); + LOG.debug("{} cache suitability: {}", exchangeId, cacheSuitability); } if (cacheSuitability == CacheSuitability.FRESH || cacheSuitability == CacheSuitability.FRESH_ENOUGH) { - LOG.debug("Cache hit is suitable"); + if (LOG.isDebugEnabled()) { + LOG.debug("{} cache hit is fresh enough", exchangeId); + } try { return convert(generateCachedResponse(request, hit.entry, now)); } catch (final ResourceIOException ex) { - if (!mayCallBackend(requestCacheControl)) { + if (requestCacheControl.isOnlyIfCached()) { + if (LOG.isDebugEnabled()) { + LOG.debug("{} request marked only-if-cached", exchangeId); + } context.setAttribute(HttpCacheContext.CACHE_RESPONSE_STATUS, CacheResponseStatus.CACHE_MODULE_RESPONSE); return convert(generateGatewayTimeout()); } @@ -277,47 +266,68 @@ private ClassicHttpResponse handleCacheHit( return chain.proceed(request, scope); } } else { - if (!mayCallBackend(requestCacheControl)) { - LOG.debug("Cache entry not is not fresh and only-if-cached requested"); + if (requestCacheControl.isOnlyIfCached()) { + if (LOG.isDebugEnabled()) { + LOG.debug("{} cache entry not is not fresh and only-if-cached requested", exchangeId); + } context.setAttribute(HttpCacheContext.CACHE_RESPONSE_STATUS, CacheResponseStatus.CACHE_MODULE_RESPONSE); return convert(generateGatewayTimeout()); } else if (cacheSuitability == CacheSuitability.MISMATCH) { - LOG.debug("Cache entry does not match the request; calling backend"); + if (LOG.isDebugEnabled()) { + LOG.debug("{} cache entry does not match the request; calling backend", exchangeId); + } return callBackend(target, request, scope, chain); } else if (request.getEntity() != null && !request.getEntity().isRepeatable()) { - LOG.debug("Request is not repeatable; calling backend"); + if (LOG.isDebugEnabled()) { + LOG.debug("{} request is not repeatable; calling backend", exchangeId); + } return callBackend(target, request, scope, chain); } else if (hit.entry.getStatus() == HttpStatus.SC_NOT_MODIFIED && !suitabilityChecker.isConditional(request)) { - LOG.debug("Non-modified cache entry does not match the non-conditional request; calling backend"); + if (LOG.isDebugEnabled()) { + LOG.debug("{} non-modified cache entry does not match the non-conditional request; calling backend", exchangeId); + } return callBackend(target, request, scope, chain); } else if (cacheSuitability == CacheSuitability.REVALIDATION_REQUIRED) { - LOG.debug("Revalidation required; revalidating cache entry"); + if (LOG.isDebugEnabled()) { + LOG.debug("{} revalidation required; revalidating cache entry", exchangeId); + } return revalidateCacheEntryWithoutFallback(responseCacheControl, hit, target, request, scope, chain); } else if (cacheSuitability == CacheSuitability.STALE_WHILE_REVALIDATED) { if (cacheRevalidator != null) { - LOG.debug("Serving stale with asynchronous revalidation"); - final String exchangeId = ExecSupport.getNextExchangeId(); - context.setExchangeId(exchangeId); + if (LOG.isDebugEnabled()) { + LOG.debug("{} serving stale with asynchronous revalidation", exchangeId); + } + final String revalidationExchangeId = ExecSupport.getNextExchangeId(); + context.setExchangeId(revalidationExchangeId); final ExecChain.Scope fork = new ExecChain.Scope( - exchangeId, + revalidationExchangeId, scope.route, scope.originalRequest, scope.execRuntime.fork(null), HttpClientContext.create()); + if (LOG.isDebugEnabled()) { + LOG.debug("{} starting asynchronous revalidation exchange {}", exchangeId, revalidationExchangeId); + } cacheRevalidator.revalidateCacheEntry( hit.getEntryKey(), () -> revalidateCacheEntry(responseCacheControl, hit, target, request, fork, chain)); context.setAttribute(HttpCacheContext.CACHE_RESPONSE_STATUS, CacheResponseStatus.CACHE_MODULE_RESPONSE); return convert(unvalidatedCacheHit(request, hit.entry)); } else { - LOG.debug("Revalidating stale cache entry (asynchronous revalidation disabled)"); + if (LOG.isDebugEnabled()) { + LOG.debug("{} revalidating stale cache entry (asynchronous revalidation disabled)", exchangeId); + } return revalidateCacheEntryWithFallback(requestCacheControl, responseCacheControl, hit, target, request, scope, chain); } } else if (cacheSuitability == CacheSuitability.STALE) { - LOG.debug("Revalidating stale cache entry"); + if (LOG.isDebugEnabled()) { + LOG.debug("{} revalidating stale cache entry", exchangeId); + } return revalidateCacheEntryWithFallback(requestCacheControl, responseCacheControl, hit, target, request, scope, chain); } else { - LOG.debug("Cache entry not usable; calling backend"); + if (LOG.isDebugEnabled()) { + LOG.debug("{} cache entry not usable; calling backend", exchangeId); + } return callBackend(target, request, scope, chain); } } @@ -357,7 +367,7 @@ ClassicHttpResponse revalidateCacheEntry( final CacheHit updated = responseCache.update(hit, target, request, backendResponse, requestDate, responseDate); return convert(generateCachedResponse(request, updated.entry, responseDate)); } - return handleBackendResponse(target, conditionalRequest, requestDate, responseDate, backendResponse); + return handleBackendResponse(scope.exchangeId, target, conditionalRequest, requestDate, responseDate, backendResponse); } catch (final IOException | RuntimeException ex) { backendResponse.close(); throw ex; @@ -371,11 +381,14 @@ ClassicHttpResponse revalidateCacheEntryWithoutFallback( final ClassicHttpRequest request, final ExecChain.Scope scope, final ExecChain chain) throws HttpException { + final String exchangeId = scope.exchangeId; final HttpClientContext context = scope.clientContext; try { return revalidateCacheEntry(responseCacheControl, hit, target, request, scope, chain); } catch (final IOException ex) { - LOG.debug(ex.getMessage(), ex); + if (LOG.isDebugEnabled()) { + LOG.debug("{} I/O error while revalidating cache entry", exchangeId, ex); + } context.setAttribute(HttpCacheContext.CACHE_RESPONSE_STATUS, CacheResponseStatus.CACHE_MODULE_RESPONSE); return convert(generateGatewayTimeout()); } @@ -389,15 +402,20 @@ ClassicHttpResponse revalidateCacheEntryWithFallback( final ClassicHttpRequest request, final ExecChain.Scope scope, final ExecChain chain) throws HttpException, IOException { + final String exchangeId = scope.exchangeId; final HttpClientContext context = scope.clientContext; final ClassicHttpResponse response; try { response = revalidateCacheEntry(responseCacheControl, hit, target, request, scope, chain); } catch (final IOException ex) { - LOG.debug(ex.getMessage(), ex); + if (LOG.isDebugEnabled()) { + LOG.debug("{} I/O error while revalidating cache entry", exchangeId, ex); + } context.setAttribute(HttpCacheContext.CACHE_RESPONSE_STATUS, CacheResponseStatus.CACHE_MODULE_RESPONSE); if (suitabilityChecker.isSuitableIfError(requestCacheControl, responseCacheControl, hit.entry, getCurrentDate())) { - LOG.debug("Serving stale response due to IOException and stale-if-error enabled"); + if (LOG.isDebugEnabled()) { + LOG.debug("{} serving stale response due to IOException and stale-if-error enabled", exchangeId); + } return convert(unvalidatedCacheHit(request, hit.entry)); } else { return convert(generateGatewayTimeout()); @@ -407,7 +425,7 @@ ClassicHttpResponse revalidateCacheEntryWithFallback( if (staleIfErrorAppliesTo(status) && suitabilityChecker.isSuitableIfError(requestCacheControl, responseCacheControl, hit.entry, getCurrentDate())) { if (LOG.isDebugEnabled()) { - LOG.debug("Serving stale response due to {} status and stale-if-error enabled", status); + LOG.debug("{} serving stale response due to {} status and stale-if-error enabled", exchangeId, status); } EntityUtils.consume(response.getEntity()); context.setAttribute(HttpCacheContext.CACHE_RESPONSE_STATUS, CacheResponseStatus.CACHE_MODULE_RESPONSE); @@ -417,6 +435,7 @@ ClassicHttpResponse revalidateCacheEntryWithFallback( } ClassicHttpResponse handleBackendResponse( + final String exchangeId, final HttpHost target, final ClassicHttpRequest request, final Instant requestDate, @@ -425,26 +444,33 @@ ClassicHttpResponse handleBackendResponse( responseCache.evictInvalidatedEntries(target, request, backendResponse); if (isResponseTooBig(backendResponse.getEntity())) { - LOG.debug("Backend response is known to be too big"); + if (LOG.isDebugEnabled()) { + LOG.debug("{} backend response is known to be too big", exchangeId); + } return backendResponse; } final ResponseCacheControl responseCacheControl = CacheControlHeaderParser.INSTANCE.parse(backendResponse); final boolean cacheable = responseCachingPolicy.isResponseCacheable(responseCacheControl, request, backendResponse); if (cacheable) { storeRequestIfModifiedSinceFor304Response(request, backendResponse); - return cacheAndReturnResponse(target, request, backendResponse, requestDate, responseDate); + return cacheAndReturnResponse(exchangeId, target, request, backendResponse, requestDate, responseDate); + } + if (LOG.isDebugEnabled()) { + LOG.debug("{} backend response is not cacheable", exchangeId); } - LOG.debug("Backend response is not cacheable"); return backendResponse; } ClassicHttpResponse cacheAndReturnResponse( + final String exchangeId, final HttpHost target, final HttpRequest request, final ClassicHttpResponse backendResponse, final Instant requestSent, final Instant responseReceived) throws IOException { - LOG.debug("Caching backend response"); + if (LOG.isDebugEnabled()) { + LOG.debug("{} caching backend response", exchangeId); + } // handle 304 Not Modified responses if (backendResponse.getCode() == HttpStatus.SC_NOT_MODIFIED) { @@ -474,7 +500,9 @@ ClassicHttpResponse cacheAndReturnResponse( buf.append(tmp, 0, l); total += l; if (total > cacheConfig.getMaxObjectSize()) { - LOG.debug("Backend response content length exceeds maximum"); + if (LOG.isDebugEnabled()) { + LOG.debug("{} backend response content length exceeds maximum", exchangeId); + } backendResponse.setEntity(new CombinedEntity(entity, buf)); return backendResponse; } @@ -489,14 +517,20 @@ ClassicHttpResponse cacheAndReturnResponse( final CacheMatch result = responseCache.match(target ,request); hit = result != null ? result.hit : null; if (HttpCacheEntry.isNewer(hit != null ? hit.entry : null, backendResponse)) { - LOG.debug("Backend already contains fresher cache entry"); + if (LOG.isDebugEnabled()) { + LOG.debug("{} backend already contains fresher cache entry", exchangeId); + } } else { hit = responseCache.store(target, request, backendResponse, buf, requestSent, responseReceived); - LOG.debug("Backend response successfully cached"); + if (LOG.isDebugEnabled()) { + LOG.debug("{} backend response successfully cached", exchangeId); + } } } else { hit = responseCache.store(target, request, backendResponse, buf, requestSent, responseReceived); - LOG.debug("Backend response successfully cached (freshness check skipped)"); + if (LOG.isDebugEnabled()) { + LOG.debug("{} backend response successfully cached (freshness check skipped)", exchangeId); + } } return convert(responseGenerator.generateResponse(request, hit.entry)); } @@ -508,13 +542,18 @@ private ClassicHttpResponse handleCacheMiss( final ClassicHttpRequest request, final ExecChain.Scope scope, final ExecChain chain) throws IOException, HttpException { - cacheMisses.getAndIncrement(); + final String exchangeId = scope.exchangeId; + if (LOG.isDebugEnabled()) { - LOG.debug("Request {} {}: cache miss", request.getMethod(), request.getRequestUri()); + LOG.debug("{} cache miss: {}", exchangeId, new RequestLine(request)); } + cacheMisses.getAndIncrement(); final HttpClientContext context = scope.clientContext; - if (!mayCallBackend(requestCacheControl)) { + if (requestCacheControl.isOnlyIfCached()) { + if (LOG.isDebugEnabled()) { + LOG.debug("{} request marked only-if-cached", exchangeId); + } context.setAttribute(HttpCacheContext.CACHE_RESPONSE_STATUS, CacheResponseStatus.CACHE_MODULE_RESPONSE); return convert(generateGatewayTimeout()); } @@ -534,6 +573,8 @@ ClassicHttpResponse negotiateResponseFromVariants( final ExecChain.Scope scope, final ExecChain chain, final List variants) throws IOException, HttpException { + final String exchangeId = scope.exchangeId; + final Map variantMap = new HashMap<>(); for (final CacheHit variant : variants) { final Header header = variant.entry.getFirstHeader(HttpHeaders.ETAG); @@ -550,7 +591,7 @@ ClassicHttpResponse negotiateResponseFromVariants( final Instant responseDate = getCurrentDate(); if (backendResponse.getCode() != HttpStatus.SC_NOT_MODIFIED) { - return handleBackendResponse(target, request, requestDate, responseDate, backendResponse); + return handleBackendResponse(exchangeId, target, request, requestDate, responseDate, backendResponse); } else { // 304 response are not expected to have an enclosed content body, but still backendResponse.close(); @@ -558,14 +599,18 @@ ClassicHttpResponse negotiateResponseFromVariants( final Header resultEtagHeader = backendResponse.getFirstHeader(HttpHeaders.ETAG); if (resultEtagHeader == null) { - LOG.warn("304 response did not contain ETag"); + if (LOG.isDebugEnabled()) { + LOG.debug("{} 304 response did not contain ETag", exchangeId); + } return callBackend(target, request, scope, chain); } final String resultEtag = resultEtagHeader.getValue(); final CacheHit match = variantMap.get(resultEtag); if (match == null) { - LOG.debug("304 response did not contain ETag matching one sent in If-None-Match"); + if (LOG.isDebugEnabled()) { + LOG.debug("{} 304 response did not contain ETag matching one sent in If-None-Match", exchangeId); + } return callBackend(target, request, scope, chain); } diff --git a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachingExecBase.java b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachingExecBase.java index 4596e02238..c2facf4ab9 100644 --- a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachingExecBase.java +++ b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachingExecBase.java @@ -40,8 +40,6 @@ import org.apache.hc.core5.http.HttpResponse; import org.apache.hc.core5.http.HttpStatus; import org.apache.hc.core5.http.Method; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; public class CachingExecBase { @@ -56,8 +54,6 @@ public class CachingExecBase { final CachedResponseSuitabilityChecker suitabilityChecker; final CacheConfig cacheConfig; - private static final Logger LOG = LoggerFactory.getLogger(CachingExecBase.class); - CachingExecBase( final CacheValidityPolicy validityPolicy, final ResponseCachingPolicy responseCachingPolicy, @@ -132,14 +128,6 @@ SimpleHttpResponse unvalidatedCacheHit(final HttpRequest request, final HttpCach return responseGenerator.generateResponse(request, entry); } - boolean mayCallBackend(final RequestCacheControl requestCacheControl) { - if (requestCacheControl.isOnlyIfCached()) { - LOG.debug("Request marked only-if-cached"); - return false; - } - return true; - } - Instant getCurrentDate() { return Instant.now(); } diff --git a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/RequestCacheControl.java b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/RequestCacheControl.java index 1df87ac40e..ef4d1bf8d5 100644 --- a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/RequestCacheControl.java +++ b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/RequestCacheControl.java @@ -137,16 +137,37 @@ public long getStaleIfError() { @Override public String toString() { - return "RequestCacheControl{" + - "maxAge=" + maxAge + - ", maxStale=" + maxStale + - ", minFresh=" + minFresh + - ", noCache=" + noCache + - ", noStore=" + noStore + - ", onlyIfCached=" + onlyIfCached + - ", staleIfError=" + staleIfError + - ", noTransform=" + noTransform + - '}'; + final StringBuilder buf = new StringBuilder(); + buf.append("["); + if (maxAge >= 0) { + buf.append("max-age=").append(maxAge).append(","); + } + if (maxStale >= 0) { + buf.append("max-stale=").append(maxStale).append(","); + } + if (minFresh >= 0) { + buf.append("max-fresh=").append(minFresh).append(","); + } + if (noCache) { + buf.append("no-cache").append(","); + } + if (noStore) { + buf.append("no-store").append(","); + } + if (onlyIfCached) { + buf.append("only-if-cached").append(","); + } + if (staleIfError >= 0) { + buf.append("stale-if-error").append(staleIfError).append(","); + } + if (noTransform) { + buf.append("no-transform").append(","); + } + if (buf.charAt(buf.length() - 1) == ',') { + buf.setLength(buf.length() - 1); + } + buf.append("]"); + return buf.toString(); } static Builder builder() { diff --git a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/ResponseCacheControl.java b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/ResponseCacheControl.java index 4e548bd4dc..cda5c58022 100644 --- a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/ResponseCacheControl.java +++ b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/ResponseCacheControl.java @@ -294,21 +294,49 @@ public boolean isImmutable() { @Override public String toString() { - return "CacheControl{" + - "maxAge=" + maxAge + - ", sharedMaxAge=" + sharedMaxAge + - ", noCache=" + noCache + - ", noStore=" + noStore + - ", cachePrivate=" + cachePrivate + - ", mustRevalidate=" + mustRevalidate + - ", proxyRevalidate=" + proxyRevalidate + - ", cachePublic=" + cachePublic + - ", staleWhileRevalidate=" + staleWhileRevalidate + - ", staleIfError=" + staleIfError + - ", noCacheFields=" + noCacheFields + - ", mustUnderstand=" + mustUnderstand + - ", immutable=" + immutable + - '}'; + final StringBuilder buf = new StringBuilder(); + buf.append("["); + if (maxAge >= 0) { + buf.append("max-age=").append(maxAge).append(","); + } + if (sharedMaxAge >= 0) { + buf.append("shared-max-age=").append(sharedMaxAge).append(","); + } + if (noCache) { + buf.append("no-cache").append(","); + } + if (noStore) { + buf.append("no-store").append(","); + } + if (cachePrivate) { + buf.append("private").append(","); + } + if (cachePublic) { + buf.append("public").append(","); + } + if (mustRevalidate) { + buf.append("must-revalidate").append(","); + } + if (proxyRevalidate) { + buf.append("proxy-revalidate").append(","); + } + if (staleWhileRevalidate >= 0) { + buf.append("state-while-revalidate=").append(staleWhileRevalidate).append(","); + } + if (staleIfError >= 0) { + buf.append("stale-if-error").append(staleIfError).append(","); + } + if (mustUnderstand) { + buf.append("must-understand").append(","); + } + if (immutable) { + buf.append("immutable").append(","); + } + if (buf.charAt(buf.length() - 1) == ',') { + buf.setLength(buf.length() - 1); + } + buf.append("]"); + return buf.toString(); } static Builder builder() { diff --git a/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestCacheableRequestPolicy.java b/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestCacheableRequestPolicy.java index 2d68671b43..b4b04190e8 100644 --- a/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestCacheableRequestPolicy.java +++ b/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestCacheableRequestPolicy.java @@ -45,7 +45,7 @@ public void testIsGetServableFromCache() { final BasicHttpRequest request = new BasicHttpRequest("GET", "someUri"); final RequestCacheControl cacheControl = RequestCacheControl.builder().build(); - Assertions.assertTrue(policy.isServableFromCache(cacheControl, request)); + Assertions.assertTrue(policy.canBeServedFromCache(cacheControl, request)); } @Test @@ -55,14 +55,14 @@ public void testIsGetWithCacheControlServableFromCache() { .setNoCache(true) .build(); - Assertions.assertFalse(policy.isServableFromCache(cacheControl, request)); + Assertions.assertFalse(policy.canBeServedFromCache(cacheControl, request)); final RequestCacheControl cacheControl2 = RequestCacheControl.builder() .setNoStore(true) .setMaxAge(20) .build(); - Assertions.assertFalse(policy.isServableFromCache(cacheControl2, request)); + Assertions.assertFalse(policy.canBeServedFromCache(cacheControl2, request)); } @Test @@ -70,13 +70,13 @@ public void testIsHeadServableFromCache() { final BasicHttpRequest request = new BasicHttpRequest("HEAD", "someUri"); final RequestCacheControl cacheControl = RequestCacheControl.builder().build(); - Assertions.assertTrue(policy.isServableFromCache(cacheControl, request)); + Assertions.assertTrue(policy.canBeServedFromCache(cacheControl, request)); final RequestCacheControl cacheControl2 = RequestCacheControl.builder() .setMaxAge(20) .build(); - Assertions.assertTrue(policy.isServableFromCache(cacheControl2, request)); + Assertions.assertTrue(policy.canBeServedFromCache(cacheControl2, request)); } @Test @@ -86,7 +86,7 @@ public void testIsHeadWithCacheControlServableFromCache() { .setNoCache(true) .build(); - Assertions.assertFalse(policy.isServableFromCache(cacheControl, request)); + Assertions.assertFalse(policy.canBeServedFromCache(cacheControl, request)); request.addHeader("Cache-Control", "no-store"); request.addHeader("Cache-Control", "max-age=20"); @@ -95,7 +95,7 @@ public void testIsHeadWithCacheControlServableFromCache() { .setMaxAge(20) .build(); - Assertions.assertFalse(policy.isServableFromCache(cacheControl2, request)); + Assertions.assertFalse(policy.canBeServedFromCache(cacheControl2, request)); } @Test @@ -104,11 +104,11 @@ public void testIsArbitraryMethodServableFromCache() { final RequestCacheControl cacheControl = RequestCacheControl.builder() .build(); - Assertions.assertFalse(policy.isServableFromCache(cacheControl, request)); + Assertions.assertFalse(policy.canBeServedFromCache(cacheControl, request)); final BasicHttpRequest request2 = new BasicHttpRequest("huh", "someUri"); - Assertions.assertFalse(policy.isServableFromCache(cacheControl, request2)); + Assertions.assertFalse(policy.canBeServedFromCache(cacheControl, request2)); } 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 961e6615eb..b1616c3311 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 @@ -900,7 +900,7 @@ public void testTooLargeResponsesAreNotCached() throws Exception { originResponse.setHeader("Date", DateUtils.formatStandardDate(responseGenerated)); originResponse.setHeader("ETag", "\"etag\""); - impl.cacheAndReturnResponse(host, request, originResponse, requestSent, responseReceived); + impl.cacheAndReturnResponse("exchange-id", host, request, originResponse, requestSent, responseReceived); Mockito.verify(cache, Mockito.never()).store( Mockito.any(), Mockito.any(), Mockito.any(), Mockito.any(), Mockito.any(), Mockito.any()); @@ -936,7 +936,7 @@ public void testSmallEnoughResponsesAreCached() throws Exception { Mockito.eq(requestSent), Mockito.eq(responseReceived))).thenReturn(new CacheHit("key", httpCacheEntry)); - impl.cacheAndReturnResponse(host, request, originResponse, requestSent, responseReceived); + impl.cacheAndReturnResponse("exchange-id", host, request, originResponse, requestSent, responseReceived); Mockito.verify(mockCache).store( Mockito.any(), @@ -1274,7 +1274,7 @@ public void testNotModifiedResponseUpdatesCacheEntry() throws Exception { .thenReturn(new CacheHit("key", cacheEntry)); // Call cacheAndReturnResponse with 304 Not Modified response - final ClassicHttpResponse cachedResponse = impl.cacheAndReturnResponse(host, request, backendResponse, requestSent, responseReceived); + final ClassicHttpResponse cachedResponse = impl.cacheAndReturnResponse("exchange-id", host, request, backendResponse, requestSent, responseReceived); // Verify cache entry is updated Mockito.verify(mockCache).update( From b69472ade0a60f9ffc6b9058f0b7c1cbfaaeed5f Mon Sep 17 00:00:00 2001 From: Oleg Kalnichevski Date: Wed, 15 Nov 2023 15:16:56 +0100 Subject: [PATCH 8/8] Bug fix: when validating a cache entry the protocol handlers must use the current request message with additional headers generated by the previous request interceptors instead of the original request message --- .../org/apache/hc/client5/http/impl/cache/AsyncCachingExec.java | 2 +- .../java/org/apache/hc/client5/http/impl/cache/CachingExec.java | 2 +- 2 files changed, 2 insertions(+), 2 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 0d7261a017..d994d4a732 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 @@ -770,7 +770,7 @@ void revalidateCacheEntry( final Instant requestDate = getCurrentDate(); final HttpRequest conditionalRequest = conditionalRequestBuilder.buildConditionalRequest( responseCacheControl, - BasicRequestBuilder.copy(scope.originalRequest).build(), + BasicRequestBuilder.copy(request).build(), hit.entry); final HttpClientContext context = scope.clientContext; chainProceed(conditionalRequest, entityProducer, scope, chain, new 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 60e9f99af7..7a42731e13 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 @@ -343,7 +343,7 @@ ClassicHttpResponse revalidateCacheEntry( final HttpClientContext context = scope.clientContext; Instant requestDate = getCurrentDate(); final ClassicHttpRequest conditionalRequest = conditionalRequestBuilder.buildConditionalRequest( - responseCacheControl, scope.originalRequest, hit.entry); + responseCacheControl, request, hit.entry); ClassicHttpResponse backendResponse = chain.proceed(conditionalRequest, scope); try {