[GitHub] [httpcomponents-client] ok2c commented on a diff in pull request #422: Implement the stale-if-error request cache directive from RFC 5861

["ok2c (via GitHub)" <gi...@apache.org>]

ok2c commented on code in PR #422:
URL: https://github.com/apache/httpcomponents-client/pull/422#discussion_r1170866895


##########
httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachingExecBase.java:
##########
@@ -370,4 +372,33 @@ void storeRequestIfModifiedSinceFor304Response(final HttpRequest request, final
         }
     }
 
+    /**
+     * 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}.
+     *
+     * @param entry the cached HTTP response entry to check
+     * @return {@code true} if a stale response can be served in case of an error status code, {@code false} otherwise
+     */
+    boolean isStaleIfErrorEnabled(final HttpCacheEntry entry) {
+        // Check if the stale-while-revalidate extension is enabled
+        if (cacheConfig.isStaleIfErrorEnabled()) {
+            // 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) {

Review Comment:
   @arturobernalg Looks mostly good. 
   1. Could you please move this logic to `ResponseCachingPolicy`? It belongs there.
   2. There are multiple places in `ResponseCachingPolicy`where `Cache-Control` header gets parsed. This looks wasteful. Could you please see if  the `CacheControl` could be produced once and re-used by multiple policy methods? You may need to pull the parsing logic from the `ResponseCachingPolicy` into the caching exec interceptor (both classic and async). 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org