You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hc.apache.org by "arturobernalg (via GitHub)" <gi...@apache.org> on 2023/03/11 19:42:16 UTC
[GitHub] [httpcomponents-client] arturobernalg opened a new pull request, #422: Implement the stale-if-error request cache directive from RFC 5861
arturobernalg opened a new pull request, #422:
URL: https://github.com/apache/httpcomponents-client/pull/422
This Pull request implements the stale-if-error request cache directive from RFC 5861 in the BasicHttpCache class. The stale-if-error directive allows clients to receive a stale cache entry when a request results in an error, rather than an error response.
To implement this, we added the cacheConfig and cacheValidityPolicy fields to the BasicHttpCache class and modified the getCacheEntry method to check if the staleIfError directive is enabled and return the stale cache entry if it is. Additionally, we updated the CacheConfig class to add a staleIfErrorEnabled boolean field to control this behavior.
Here's a summary of the changes:
Added cacheConfig and cacheValidityPolicy fields to BasicHttpCache
Modified getCacheEntry to check for the staleIfError directive and return the stale cache entry if enabled
Added staleIfErrorEnabled boolean field to CacheConfig
Please let me know if there are any further changes or modifications required.
--
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
[GitHub] [httpcomponents-client] arturobernalg commented on pull request #422: Implement the stale-if-error request cache directive from RFC 5861
Posted by "arturobernalg (via GitHub)" <gi...@apache.org>.
arturobernalg commented on PR #422:
URL: https://github.com/apache/httpcomponents-client/pull/422#issuecomment-1516152337
> @arturobernalg Looks good. I can merge this change-set unless you also want to add handling of I/O errors in the same PR.
@ok2c
Could you please clarify which part of the codebase you are referring to for this change?
--
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
[GitHub] [httpcomponents-client] ok2c commented on pull request #422: Implement the stale-if-error request cache directive from RFC 5861
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on PR #422:
URL: https://github.com/apache/httpcomponents-client/pull/422#issuecomment-1517399771
> > @arturobernalg Looks good. I can merge this change-set unless you also want to add handling of I/O errors in the same PR.
>
> @ok2c Could you please clarify which part of the codebase you are referring to for this change?
```
The stale-if-error HTTP Cache-Control extension allows a cache to
return a stale response when an error -- e.g., a 500 Internal Server
Error, a network segment, or DNS failure -- is encountered, rather
than returning a "hard" error. This improves availability.
```
The change-set introduces `stale-if-error` support for HTTP status 5xx but not for network failures. You do not have to do it. I am going to merge the PR anyway. I am just wondering if you were planning to work on it as well.
--
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
[GitHub] [httpcomponents-client] ok2c merged pull request #422: Implement the stale-if-error request cache directive from RFC 5861
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c merged PR #422:
URL: https://github.com/apache/httpcomponents-client/pull/422
--
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
[GitHub] [httpcomponents-client] ok2c commented on a diff in pull request #422: Implement the stale-if-error request cache directive from RFC 5861
Posted by "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
[GitHub] [httpcomponents-client] arturobernalg commented on pull request #422: Implement the stale-if-error request cache directive from RFC 5861
Posted by "arturobernalg (via GitHub)" <gi...@apache.org>.
arturobernalg commented on PR #422:
URL: https://github.com/apache/httpcomponents-client/pull/422#issuecomment-1517428956
> > > @arturobernalg Looks good. I can merge this change-set unless you also want to add handling of I/O errors in the same PR.
> >
> >
> > @ok2c Could you please clarify which part of the codebase you are referring to for this change?
>
> ```
> The stale-if-error HTTP Cache-Control extension allows a cache to
> return a stale response when an error -- e.g., a 500 Internal Server
> Error, a network segment, or DNS failure -- is encountered, rather
> than returning a "hard" error. This improves availability.
> ```
>
> The change-set introduces `stale-if-error` support for HTTP status 5xx but not for network failures. You do not have to do it. I am going to merge the PR anyway. I am just wondering if you were planning to work on it as well.
I see. i'll take a look this afternoon. I'll do the change in another PR
TY
--
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