cache: fix excessive caching and some other lack of caching #4335
Conversation
307 responses must not be cached if they do not have an explicit caching directive. Final responses with an explicit caching directive can be cached even if their status code is not heuristically cacheable status codes. fix part of nodejs#4334
finish fixing nodejs#4334
26fe391 to
00d1d45
Compare
|
@fredericDelaporte could you check if there is any docs that needs updating? |
|
I have put a "skipped" in the "Documented" item because I considered no documentation update was required. (Nothing documented about cache has changed, and the change is bug fixes and a slight improvement. I have also checked no mention of 307 being specifically cached were there.) Maybe I should have instead ticked it for meaning I have done these checks? |
lib/handler/cache-handler.js
Outdated
| if (cacheByDefault) { | ||
| // When cacheByDefault is truthy, the cache interceptor does no more mandate an explicit expiry prior to | ||
| // reaching here. We should then check the full condition allowing to consider caching responses. | ||
| // Responses with neither status codes that are heuristically cacheable, nor "explicit enough" caching | ||
| // directives, are not cacheable. "Explicit enough": see https://www.rfc-editor.org/rfc/rfc9111.html#section-3 | ||
| if (!HEURISTICALLY_CACHEABLE_STATUS_CODES.includes(statusCode) && !resHeaders['expires'] && | ||
| !cacheControlDirectives.public && | ||
| cacheControlDirectives['max-age'] === undefined && | ||
| // RFC 9111: a private response directive, if the cache is not shared | ||
| !(cacheControlDirectives.private && cacheType === 'private') && | ||
| !(cacheControlDirectives['s-maxage'] !== undefined && cacheType === 'shared') | ||
| ) { | ||
| return false | ||
| } | ||
| } |
There was a problem hiding this comment.
When I considered the subject, I was not expecting it to turn that way because I was not aware of the cacheByDefault option.
Due to the code preceding the call to canCacheResponse, which actually does already some cacheability checks, we need that big test only when cacheByDefault is truthy.
Maybe that code should be moved into canCacheResponse instead of staying out of it, allowing to have a better picture of how the cache interceptor checks cacheability.
|
The remaining CI failures seem to be a flaky websockets test. (There is a flaky cache test too which has blown on my setup once, name "revalidates the request when the response is stale".) |
This reverts commit 5cf41d4.
|
Is there is something more I should do for this PR? |
307 responses must not be cached if they do not have an explicit caching directive.
Final responses with an explicit caching directive can be cached even if their status code is not heuristically cacheable status codes.
206 and 304 semantics are not handled by the cache and as such must not be cached.
This relates to...
fix #4334.
Changes
Features
N/A
Bug Fixes
Breaking Changes and Deprecations
N/A
Status