From f24c8010bbff1ba23063152a3bcd5089d8d3a954 Mon Sep 17 00:00:00 2001 From: Oleg Kalnichevski Date: Mon, 1 Jan 2024 13:18:53 +0100 Subject: [PATCH] Made caching of 403 responses by the classic and async protocol handlers more consistent --- .../http/impl/cache/AsyncCachingExec.java | 169 +++++++++--------- .../client5/http/impl/cache/CachingExec.java | 21 +-- 2 files changed, 97 insertions(+), 93 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 41fc2c368..2d25b204f 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 @@ -515,21 +515,20 @@ public void cancelled() { final ResponseCacheControl responseCacheControl = CacheControlHeaderParser.INSTANCE.parse(backendResponse); final boolean cacheable = responseCachingPolicy.isResponseCacheable(responseCacheControl, request, backendResponse); if (cacheable) { - cachingConsumerRef.set(new CachingAsyncDataConsumer(exchangeId, asyncExecCallback, backendResponse, entityDetails)); storeRequestIfModifiedSinceFor304Response(request, backendResponse); - } else { - if (LOG.isDebugEnabled()) { - LOG.debug("{} backend response is not cacheable", exchangeId); - } - } - final CachingAsyncDataConsumer cachingDataConsumer = cachingConsumerRef.get(); - if (cachingDataConsumer != null) { if (LOG.isDebugEnabled()) { LOG.debug("{} caching backend response", exchangeId); } + final CachingAsyncDataConsumer cachingDataConsumer = new CachingAsyncDataConsumer( + exchangeId, asyncExecCallback, backendResponse, entityDetails); + cachingConsumerRef.set(cachingDataConsumer); return cachingDataConsumer; + } else { + if (LOG.isDebugEnabled()) { + LOG.debug("{} backend response is not cacheable", exchangeId); + } + return asyncExecCallback.handleResponse(backendResponse, entityDetails); } - return asyncExecCallback.handleResponse(backendResponse, entityDetails); } @Override @@ -576,13 +575,87 @@ public void cancelled() { } + void triggerCachedResponse(final HttpCacheEntry entry) { + try { + final SimpleHttpResponse cacheResponse = responseGenerator.generateResponse(request, entry); + triggerResponse(cacheResponse, scope, asyncExecCallback); + } catch (final ResourceIOException ex) { + asyncExecCallback.failed(ex); + } + } + @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); - final HttpResponse backendResponse = cachingDataConsumer.backendResponse; + if (cachingDataConsumer == null || cachingDataConsumer.writtenThrough.get()) { + asyncExecCallback.completed(); + return; + } + final HttpResponse backendResponse = cachingDataConsumer.backendResponse; + final ByteArrayBuffer buffer = cachingDataConsumer.bufferRef.getAndSet(null); + + // Handle 304 Not Modified responses + if (backendResponse.getCode() == HttpStatus.SC_NOT_MODIFIED) { + responseCache.match(target, request, new FutureCallback() { + + @Override + 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", exchangeId); + } + responseCache.update( + hit, + target, + request, + backendResponse, + requestDate, + responseDate, + new FutureCallback() { + + @Override + public void completed(final CacheHit updated) { + if (LOG.isDebugEnabled()) { + LOG.debug("{} cache entry updated, generating response from updated entry", exchangeId); + } + triggerCachedResponse(updated.entry); + } + @Override + public void failed(final Exception cause) { + if (LOG.isDebugEnabled()) { + LOG.debug("{} request failed: {}", exchangeId, cause.getMessage()); + } + asyncExecCallback.failed(cause); + } + + @Override + public void cancelled() { + if (LOG.isDebugEnabled()) { + LOG.debug("{} cache entry updated aborted", exchangeId); + } + asyncExecCallback.failed(new InterruptedIOException()); + } + + }); + } else { + triggerNewCacheEntryResponse(backendResponse, responseDate, buffer); + } + } + + @Override + public void failed(final Exception cause) { + asyncExecCallback.failed(cause); + } + + @Override + public void cancelled() { + asyncExecCallback.failed(new InterruptedIOException()); + } + + }); + } else { if (cacheConfig.isFreshnessCheckEnabled()) { final CancellableDependency operation = scope.cancellableDependency; operation.setDependency(responseCache.match(target, request, new FutureCallback() { @@ -594,12 +667,7 @@ public void completed(final CacheMatch result) { 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); - } catch (final ResourceIOException ex) { - asyncExecCallback.failed(ex); - } + triggerCachedResponse(hit.entry); } else { triggerNewCacheEntryResponse(backendResponse, responseDate, buffer); } @@ -619,8 +687,6 @@ public void cancelled() { } else { triggerNewCacheEntryResponse(backendResponse, responseDate, buffer); } - } else { - asyncExecCallback.completed(); } } @@ -1183,69 +1249,6 @@ public AsyncDataConsumer handleResponse( final EntityDetails entityDetails) throws HttpException, IOException { final Instant responseDate = getCurrentDate(); final AsyncExecCallback callback; - // Handle 304 Not Modified responses - if (backendResponse.getCode() == HttpStatus.SC_NOT_MODIFIED) { - responseCache.match(target, request, new FutureCallback() { - @Override - 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", exchangeId); - } - responseCache.update( - hit, - target, - request, - backendResponse, - requestDate, - responseDate, - new FutureCallback() { - - @Override - public void completed(final CacheHit updated) { - try { - if (LOG.isDebugEnabled()) { - LOG.debug("{} cache entry updated, generating response from updated entry", exchangeId); - } - final SimpleHttpResponse cacheResponse = responseGenerator.generateResponse(request, updated.entry); - triggerResponse(cacheResponse, scope, asyncExecCallback); - } catch (final ResourceIOException ex) { - asyncExecCallback.failed(ex); - } - } - @Override - public void failed(final Exception cause) { - if (LOG.isDebugEnabled()) { - LOG.debug("{} request failed: {}", exchangeId, cause.getMessage()); - } - asyncExecCallback.failed(cause); - } - - @Override - public void cancelled() { - if (LOG.isDebugEnabled()) { - LOG.debug("{} cache entry updated aborted", exchangeId); - } - asyncExecCallback.failed(new InterruptedIOException()); - } - - }); - } - } - - @Override - public void failed(final Exception cause) { - asyncExecCallback.failed(cause); - } - - @Override - public void cancelled() { - asyncExecCallback.failed(new InterruptedIOException()); - } - }); - } - if (backendResponse.getCode() != HttpStatus.SC_NOT_MODIFIED) { callback = new BackendResponseHandler(target, request, requestDate, responseDate, scope, asyncExecCallback); } else { 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 c83ac89fd..f451a589d 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 @@ -454,12 +454,16 @@ ClassicHttpResponse handleBackendResponse( final boolean cacheable = responseCachingPolicy.isResponseCacheable(responseCacheControl, request, backendResponse); if (cacheable) { storeRequestIfModifiedSinceFor304Response(request, backendResponse); + if (LOG.isDebugEnabled()) { + LOG.debug("{} caching backend response", exchangeId); + } return cacheAndReturnResponse(exchangeId, target, request, backendResponse, requestDate, responseDate); + } else { + if (LOG.isDebugEnabled()) { + LOG.debug("{} backend response is not cacheable", exchangeId); + } + return backendResponse; } - if (LOG.isDebugEnabled()) { - LOG.debug("{} backend response is not cacheable", exchangeId); - } - return backendResponse; } ClassicHttpResponse cacheAndReturnResponse( @@ -469,12 +473,9 @@ ClassicHttpResponse cacheAndReturnResponse( final ClassicHttpResponse backendResponse, final Instant requestSent, final Instant responseReceived) throws IOException { - if (LOG.isDebugEnabled()) { - LOG.debug("{} caching backend response", exchangeId); - } - + final int statusCode = backendResponse.getCode(); // handle 304 Not Modified responses - if (backendResponse.getCode() == HttpStatus.SC_NOT_MODIFIED) { + if (statusCode == HttpStatus.SC_NOT_MODIFIED) { final CacheMatch result = responseCache.match(target ,request); final CacheHit hit = result != null ? result.hit : null; if (hit != null) { @@ -514,7 +515,7 @@ ClassicHttpResponse cacheAndReturnResponse( backendResponse.close(); CacheHit hit; - if (cacheConfig.isFreshnessCheckEnabled()) { + if (cacheConfig.isFreshnessCheckEnabled() && statusCode != HttpStatus.SC_NOT_MODIFIED) { final CacheMatch result = responseCache.match(target ,request); hit = result != null ? result.hit : null; if (HttpCacheEntry.isNewer(hit != null ? hit.entry : null, backendResponse)) {