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/04/19 22:02:49 UTC
[GitHub] [httpcomponents-client] arturobernalg opened a new pull request, #433: Fix invalid cache entry for HEAD requests with 304 responses
arturobernalg opened a new pull request, #433:
URL: https://github.com/apache/httpcomponents-client/pull/433
This PR addresses the issue where cache entries created by HEAD requests with null response bodies are incorrectly used for subsequent GET requests
Changes in this PR:
- Add a check in `CachedResponseSuitabilityChecker` to ensure that cache entries created by HEAD requests are not used to serve GET requests.
- Properly handle 304 Not Modified responses in `CachingExec` by updating the existing cache entry.
- Add relevant tests to validate the fix.
--
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 a diff in pull request #433: Fix invalid cache entry for HEAD requests with 304 responses
Posted by "arturobernalg (via GitHub)" <gi...@apache.org>.
arturobernalg commented on code in PR #433:
URL: https://github.com/apache/httpcomponents-client/pull/433#discussion_r1176775525
##########
httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachingExec.java:
##########
@@ -395,6 +427,22 @@ ClassicHttpResponse cacheAndReturnResponse(
final Instant requestSent,
final Instant responseReceived) throws IOException {
LOG.debug("Caching backend response");
+
+ // handle 304 Not Modified responses
+ if (backendResponse.getCode() == HttpStatus.SC_NOT_MODIFIED) {
Review Comment:
Done.
https://github.com/apache/httpcomponents-client/pull/438
--
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 #433: Handle 304 Not Modified responses
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c merged PR #433:
URL: https://github.com/apache/httpcomponents-client/pull/433
--
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 a diff in pull request #433: Fix invalid cache entry for HEAD requests with 304 responses
Posted by "arturobernalg (via GitHub)" <gi...@apache.org>.
arturobernalg commented on code in PR #433:
URL: https://github.com/apache/httpcomponents-client/pull/433#discussion_r1175170896
##########
httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachingExec.java:
##########
@@ -395,6 +427,22 @@ ClassicHttpResponse cacheAndReturnResponse(
final Instant requestSent,
final Instant responseReceived) throws IOException {
LOG.debug("Caching backend response");
+
+ // handle 304 Not Modified responses
+ if (backendResponse.getCode() == HttpStatus.SC_NOT_MODIFIED) {
Review Comment:
regarding with the unrelated change I think the only one its the remove method.
basically the change are:
1. In the CachingExec class, handle 304 Not Modified responses in the cacheAndReturnResponse method.
2. In the CachedResponseSuitabilityChecker class, I added a new method isGetRequestWithHeadCacheEntry and a check in the canCachedResponseBeUsed method.
--
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 #433: Fix invalid cache entry for HEAD requests with 304 responses
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on code in PR #433:
URL: https://github.com/apache/httpcomponents-client/pull/433#discussion_r1177456186
##########
httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachingExec.java:
##########
@@ -395,6 +427,22 @@ ClassicHttpResponse cacheAndReturnResponse(
final Instant requestSent,
final Instant responseReceived) throws IOException {
LOG.debug("Caching backend response");
+
+ // handle 304 Not Modified responses
+ if (backendResponse.getCode() == HttpStatus.SC_NOT_MODIFIED) {
Review Comment:
@arturobernalg #438 merged.
--
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 a diff in pull request #433: Fix invalid cache entry for HEAD requests with 304 responses
Posted by "arturobernalg (via GitHub)" <gi...@apache.org>.
arturobernalg commented on code in PR #433:
URL: https://github.com/apache/httpcomponents-client/pull/433#discussion_r1174554490
##########
httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachedResponseSuitabilityChecker.java:
##########
@@ -241,6 +246,26 @@ public boolean canCachedResponseBeUsed(final HttpHost host, final HttpRequest re
return true;
}
+ /**
+ * Determine if I can utilize a {@link HttpCacheEntry} to respond to the given
+ * {@link HttpRequest}
+ *
+ * @param host
+ * {@link HttpHost}
+ * @param request
+ * {@link HttpRequest}
+ * @param entry
+ * {@link HttpCacheEntry}
+ * @param now
+ * Right now in time
+ * @return boolean yes/no answer
+ * @deprecated (5.3) use {@link #canCachedResponseBeUsed(HttpRequest, HttpCacheEntry, Instant)}
+ */
+ @Deprecated
Review Comment:
@ok2c True,
I just remove the old method.
--
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 #433: Fix invalid cache entry for HEAD requests with 304 responses
Posted by "arturobernalg (via GitHub)" <gi...@apache.org>.
arturobernalg commented on PR #433:
URL: https://github.com/apache/httpcomponents-client/pull/433#issuecomment-1517495570
> @arturobernalg Is there a JIRA associated with this PR?
https://issues.apache.org/jira/browse/HTTPCLIENT-1920
--
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 a diff in pull request #433: Fix invalid cache entry for HEAD requests with 304 responses
Posted by "arturobernalg (via GitHub)" <gi...@apache.org>.
arturobernalg commented on code in PR #433:
URL: https://github.com/apache/httpcomponents-client/pull/433#discussion_r1178151402
##########
httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachingExec.java:
##########
@@ -395,6 +427,22 @@ ClassicHttpResponse cacheAndReturnResponse(
final Instant requestSent,
final Instant responseReceived) throws IOException {
LOG.debug("Caching backend response");
+
+ // handle 304 Not Modified responses
+ if (backendResponse.getCode() == HttpStatus.SC_NOT_MODIFIED) {
Review Comment:
TY,
@ok2c conflicts resolved.
--
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 #433: Fix invalid cache entry for HEAD requests with 304 responses
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on code in PR #433:
URL: https://github.com/apache/httpcomponents-client/pull/433#discussion_r1176101961
##########
httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachingExec.java:
##########
@@ -395,6 +427,22 @@ ClassicHttpResponse cacheAndReturnResponse(
final Instant requestSent,
final Instant responseReceived) throws IOException {
LOG.debug("Caching backend response");
+
+ // handle 304 Not Modified responses
+ if (backendResponse.getCode() == HttpStatus.SC_NOT_MODIFIED) {
Review Comment:
@arturobernalg The points 1 and 2 do not look logically related to me. I would prefer the change-set split into two separate ones but I will not insist. I can merge the PR as is.
--
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 #433: Fix invalid cache entry for HEAD requests with 304 responses
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on code in PR #433:
URL: https://github.com/apache/httpcomponents-client/pull/433#discussion_r1174524383
##########
httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachedResponseSuitabilityChecker.java:
##########
@@ -241,6 +246,26 @@ public boolean canCachedResponseBeUsed(final HttpHost host, final HttpRequest re
return true;
}
+ /**
+ * Determine if I can utilize a {@link HttpCacheEntry} to respond to the given
+ * {@link HttpRequest}
+ *
+ * @param host
+ * {@link HttpHost}
+ * @param request
+ * {@link HttpRequest}
+ * @param entry
+ * {@link HttpCacheEntry}
+ * @param now
+ * Right now in time
+ * @return boolean yes/no answer
+ * @deprecated (5.3) use {@link #canCachedResponseBeUsed(HttpRequest, HttpCacheEntry, Instant)}
+ */
+ @Deprecated
Review Comment:
@arturobernalg This class is package private. There is no need deprecate old methods. Just remove them.
--
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 a diff in pull request #433: Fix invalid cache entry for HEAD requests with 304 responses
Posted by "arturobernalg (via GitHub)" <gi...@apache.org>.
arturobernalg commented on code in PR #433:
URL: https://github.com/apache/httpcomponents-client/pull/433#discussion_r1175139334
##########
httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachingExec.java:
##########
@@ -395,6 +427,22 @@ ClassicHttpResponse cacheAndReturnResponse(
final Instant requestSent,
final Instant responseReceived) throws IOException {
LOG.debug("Caching backend response");
+
+ // handle 304 Not Modified responses
+ if (backendResponse.getCode() == HttpStatus.SC_NOT_MODIFIED) {
Review Comment:
@ok2c
AFAIU 304 Not Modified responses is already properly implemented in AsyncCachingExec class. The code in AsyncCachingExec takes care of updating the cache and sending the appropriate response to the client when a 304 Not Modified response is received from the backend.
--
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 a diff in pull request #433: Fix invalid cache entry for HEAD requests with 304 responses
Posted by "arturobernalg (via GitHub)" <gi...@apache.org>.
arturobernalg commented on code in PR #433:
URL: https://github.com/apache/httpcomponents-client/pull/433#discussion_r1176248138
##########
httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachingExec.java:
##########
@@ -395,6 +427,22 @@ ClassicHttpResponse cacheAndReturnResponse(
final Instant requestSent,
final Instant responseReceived) throws IOException {
LOG.debug("Caching backend response");
+
+ // handle 304 Not Modified responses
+ if (backendResponse.getCode() == HttpStatus.SC_NOT_MODIFIED) {
Review Comment:
@ok2c I'll split the into two commit later this day.
--
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 #433: Fix invalid cache entry for HEAD requests with 304 responses
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on PR #433:
URL: https://github.com/apache/httpcomponents-client/pull/433#issuecomment-1523932693
@arturobernalg Please, once more for my understanding. Why do you think that `AsyncCachingExec` is not affected and does not require a symmetric change, because it looks like it does?
--
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 #433: Handle 304 Not Modified responses
Posted by "arturobernalg (via GitHub)" <gi...@apache.org>.
arturobernalg commented on PR #433:
URL: https://github.com/apache/httpcomponents-client/pull/433#issuecomment-1526042613
> @arturobernalg The title and the description of the PR look wrong now. HTTPCLIENT-1920 has been fixed by #438. Could you please update the title and the description?
updated
--
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 #433: Fix invalid cache entry for HEAD requests with 304 responses
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on code in PR #433:
URL: https://github.com/apache/httpcomponents-client/pull/433#discussion_r1174861551
##########
httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachingExec.java:
##########
@@ -395,6 +427,22 @@ ClassicHttpResponse cacheAndReturnResponse(
final Instant requestSent,
final Instant responseReceived) throws IOException {
LOG.debug("Caching backend response");
+
+ // handle 304 Not Modified responses
+ if (backendResponse.getCode() == HttpStatus.SC_NOT_MODIFIED) {
Review Comment:
@arturobernalg What about `AsyncCachingExec`? Should not it be doing the same? I also have a feeling that this PR contains two unrelated changes. It would better to separate them into distinct change-sets.
--
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 #433: Fix invalid cache entry for HEAD requests with 304 responses
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on PR #433:
URL: https://github.com/apache/httpcomponents-client/pull/433#issuecomment-1517450421
@arturobernalg Is there a JIRA associated with this PR?
--
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 #433: Fix invalid cache entry for HEAD requests with 304 responses
Posted by "arturobernalg (via GitHub)" <gi...@apache.org>.
arturobernalg commented on PR #433:
URL: https://github.com/apache/httpcomponents-client/pull/433#issuecomment-1524028901
> @arturobernalg Please, once more for my understanding. Why do you think that `AsyncCachingExec` is not affected and does not require a symmetric change, because it looks like it does?
@ok2c
You're right. my bad. Added support in asynchronous method.
--
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 #433: HTTPCLIENT-1920: Fix invalid cache entry for HEAD requests with 304 responses
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on PR #433:
URL: https://github.com/apache/httpcomponents-client/pull/433#issuecomment-1525919030
@arturobernalg The title and the description of the PR look wrong now. HTTPCLIENT-1920 has been fixed by #438. Could you please update the title and the description?
--
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