Fix HTTPCLIENT-2354: Allow caching of responses with "must-revalidate, max-age=0" in shared caches #609
Conversation
|
@arturobernalg I must admit it is difficult for me to understand the intent of your changes. You moved some things around and I do not quite understand why. Could you split your changes in two commits, one with refactoring without protocol changes and another one with only protocol changes? |
|
Or more simply, can the PR be formulated to contain only required changes without subjective changes. |
@ok2c @garydgregory the purpose of moving the code is to ensure the existing tests pass with the new validation we introduced. Without this adjustment, some of the previous tests would fail due to the added checks for "must-revalidate" and caching responses with Authorization headers. |
@arturobernalg That sounds even more worrying. What we probably need to do first is to move things around, understand why some tests are failing, fix them, and only then introduce protocol changes. |
| @@ -248,6 +256,9 @@ protected boolean isExplicitlyCacheable(final ResponseCacheControl cacheControl, | |||
| if (response.containsHeader(HttpHeaders.EXPIRES)) { | |||
| return true; | |||
| } | |||
| if (cacheControl.getMaxAge() == 0 && cacheControl.isMustRevalidate()) { | |||
There was a problem hiding this comment.
@arturobernalg Where in RFC 9111 is this behavior defined? Why does max-age need to be zero and not one, for instance?
There was a problem hiding this comment.
@ok2c The reason max-age=0 is required is that it explicitly marks the response as immediately stale, triggering revalidation on every request. A value of 1 would permit reuse for that duration, bypassing the strict revalidation requirement.
There was a problem hiding this comment.
@arturobernalg But what section of RFC requires or defines this behavior?
There was a problem hiding this comment.
@ok2c
Section 3.5 allows a shared cache to reuse responses with an Authorization header if directives like must-revalidate are present.
Section 5.2.2.2 states that when must-revalidate is set, the cache MUST NOT reuse the response once stale, requiring immediate validation.
Setting max-age=0 forces the response to become stale immediately, triggering revalidation as required by must-revalidate. This approach minimizes ambiguity but admittedly, RFC 9111 doesn't explicitly prescribe max-age=0 over max-age=1, leaving some room for interpretation regarding minimal reuse windows.
There was a problem hiding this comment.
@arturobernalg Fine by me but I see no mentioning of max-age=0 anywhere in that section. The only change I would consider reasonable based on my interpretation of the section is this:
ok2c@f4a1870
There was a problem hiding this comment.
@ok2c Looks reasonable to me as it directly addresses the core condition around Authorization and shared caches
…g of responses with "must-revalidate, max-age=0" in shared caches with Authorization headers. The change aligns with RFC 9111 Section 5.2.2.2, ensuring responses with "must-revalidate," "s-maxage," or "public" directives are cacheable. This addresses cases where responses with Authorization headers were unnecessarily excluded from caching.
arturobernalg
force-pushed
the
HTTPCLIENT-2354
branch
from
74ba76e to
843ff21
Compare
@ok2c Should I cherry-pick this to 5.4.x? |
|
@arturobernalg I would have waited with merging until some feedback from the original reporter, but it is too late now. Please go ahead and cherry-pick it to |
…g of responses with "must-revalidate, max-age=0" in shared caches with Authorization headers. The change aligns with RFC 9111 Section 5.2.2.2, ensuring responses with "must-revalidate," "s-maxage," or "public" directives are cacheable. This addresses cases where responses with Authorization headers were unnecessarily excluded from caching. (#609) (cherry picked from commit 8b1ee82)
You're absolutely right—apologies for missing your comment earlier. The changes have now been cherry-picked to 5.4.x. Thank you for pointing it out! |
Fix HTTPCLIENT-2354 by updating
ResponseCachingPolicyto handle caching of responses withmust-revalidate, max-age=0in shared caches. UpdatedisExplicitlyCacheableto account formust-revalidatewhenmax-age=0, ensuring compliance with RFC 9111. Adjusted isResponseCacheable to permit caching of responses containing Authorization headers ifmust-revalidate, s-maxage, or public directives are present. Enhanced debug logging to reflect caching decisions. Added unit tests to verify that responses withmust-revalidateand max-age=0 are cacheable in shared caches even with Authorization headers. This fix resolves the issue preventing compliant caching and aligns HttpClient behavior with RFC 9111 specifications.