fix: 304 not modified reply upon revalidation did not update cache.#4617
fix: 304 not modified reply upon revalidation did not update cache.#4617mcollina merged 5 commits intonodejs:mainfrom
304 not modified reply upon revalidation did not update cache.#4617Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4617 +/- ##
==========================================
- Coverage 92.95% 92.93% -0.02%
==========================================
Files 106 106
Lines 33015 33091 +76
==========================================
+ Hits 30688 30753 +65
- Misses 2327 2338 +11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…both etag and no etag scenarios
…wnStreamOnHeaders call.
| } | ||
| }) | ||
|
|
||
| test('stale responses are revalidated before deleteAt (if-modified-since)', async () => { |
There was a problem hiding this comment.
I feel this is covered by the other tests already (incl the ones I've added).
Also, on line 491 and 627 there's conditional requests that behave different now: they update the cache too. The scenario in these tests are thus now not entirely valid anymore.
If you prefer I could modify and reinstate them.
I do feel the organisation of the cache/revalidation tests can be improved, as we have both these tests and the ones in test/interceptors/cache-revalidate-stale.js - shouldn't we merge these files?
|
@metcoder95 ptal |
This relates to...
#4596
Rationale
When a 304 was returned during revalidation, the cache was not updated to reflect this. Instead, the cached value was kept as-is, eventually expiring. This is particularly a problem for anyone using stale-while-revalidate, as in the entire stale-while-revalidate window there'll be no updates while loading the backend with (async) revalidations.
To handle a 304, the cached body and headers will be re-used and re-inserted into cache. This is not the most optimal way, but the only way I feel is possible without breaking changes. A better way would be to require support for an
updatemethod in the cacheStore to only update the headers/metadata and keep body as-is. I didn't want to go there without discussing it first.Changes
304 not modifiedreplies will update the cache with new headers, cache times etc., but will re-use original statuscode/message, etag and body. (see changes in lib/handler/cache-handler.js)if-modified-sinceorif-none-match. These will trigger a synchronous revalidation flow.stale responses are revalidated before deleteAt (if-modified-since)andstale responses are revalidated before deleteAt (if-none-match)fromtest/interceptors/cache.jsas these did not contain the actual expected behavior. Instead I've addedrevalidates the request, handles 304s during stale-while-revalidateintest/interceptors/cache-revalidate-stale.js.Bug Fixes
See #4596
Breaking Changes and Deprecations
None
Status