feat: http caching #3562

flakey5 · 2024-09-07T06:16:04Z

Implements bare-bones opt-in http caching as per rfc9111. Bare-bones in this case means what's required by the spec and a few extra bits, with more coming in future prs.

~~Opening as a draft since there's still some more work to be done (mostly tests, but a bit more functionality-wise)~~

No request cache directives are supported at this time, this will come later.

Response caching directives supported:

public
private
s-maxage
max-age
Expires header
no-cache
no-store
stale-while-revalidate

This relates to...

Closes #3231
Closes #2760
Closes #2256
Closes #1146

Changes

Features

Opt-in http caching
In-memory default cache store

Bug Fixes

n/a

Breaking Changes and Deprecations

n/a

Status

lib/cache/lru-cache-store.js

lib/handler/cache-handler.js

ronag · 2024-09-07T07:44:21Z

Would it maybe be better to use jsdoc types rather than separate type definition files? @mcollina

index.js

lib/cache/lru-cache-store.js

lib/handler/cache-handler.js

lib/interceptor/cache.js

mcollina · 2024-09-09T05:45:12Z

Would it maybe be better to use jsdoc types rather than separate type definition files? @mcollina

I think using type definitions are in line with the rest of the codebase, and therefore the correct implementation.

If we want to migrate to a jsdoc-generated world, let's discuss in another issue!

lib/handler/cache-handler.js

lib/cache/lru-cache-store.js

lib/handler/cache-handler.js

ronag · 2024-09-11T08:29:38Z

@flakey5 please make sure to add me and @IsakT as co-authors (assuming you took some of our code in nxt-undici).

lib/cache/lru-cache-store.js

lib/handler/cache-handler.js

Implements bare-bones http caching as per rfc9111 Closes nodejs#3231 Closes nodejs#2760 Closes nodejs#2256 Closes nodejs#1146 Co-authored-by: Carlos Fuentes <[email protected]> Co-authored-by: Robert Nagy <[email protected]> Co-authored-by: Isak Törnros <[email protected]> Signed-off-by: flakey5 <[email protected]>

Signed-off-by: flakey5 <[email protected]>

types/interceptors.d.ts

test/cache-interceptor/cache-stores.js

test/cache-interceptor/interceptor.js

Signed-off-by: flakey5 <[email protected]>

lib/cache/memory-cache-store.js

lib/handler/cache-handler.js

Signed-off-by: flakey5 <[email protected]>

Co-authored-by: Robert Nagy <[email protected]>

lib/cache/memory-cache-store.js

lib/handler/cache-handler.js

lib/handler/cache-revalidation-handler.js

lib/interceptor/cache.js

test/cache-interceptor/interceptor.js

Co-authored-by: Carlos Fuentes <[email protected]>

Signed-off-by: flakey5 <[email protected]>

lib/cache/memory-cache-store.js

types/cache-interceptor.d.ts

Uzlopak

headers could be null or undefined

lib/cache/memory-cache-store.js

Uzlopak

@ronag
Should we use fastTimers?

lib/cache/memory-cache-store.js

Uzlopak

found some issue, which need some love ;)

Co-authored-by: Aras Abbasi <[email protected]>

Signed-off-by: flakey5 <[email protected]>

Co-authored-by: Aras Abbasi <[email protected]>

Signed-off-by: flakey5 <[email protected]>

Uzlopak · 2024-10-14T08:34:18Z

I will review it shortly ;)

SukkaW · 2024-10-14T13:16:48Z

lib/interceptor/cache.js

+            // https://www.rfc-editor.org/rfc/rfc9111.html#name-age
+            const age = Math.round((Date.now() - value.cachedAt) / 1000)
+
+            value.rawHeaders.push(AGE_HEADER, Buffer.from(`${age}`))


Suggested change

value.rawHeaders.push(AGE_HEADER, Buffer.from(`${age}`))

value.rawHeaders.push(AGE_HEADER, Buffer.from(age))

SukkaW · 2024-10-14T13:17:19Z

lib/interceptor/cache.js

+const CacheRevalidationHandler = require('../handler/cache-revalidation-handler')
+const { UNSAFE_METHODS } = require('../util/cache.js')
+
+const AGE_HEADER = Buffer.from('age')


I am not entirely sure why age here is lowercased. If it is Age, we can re-use getWellKnownHeaderBuffer.

ronag · 2024-10-14T13:33:10Z

Guys. Please stop with the nits for now and open follow up PRs.we need to land this.

lib/handler/cache-revalidation-handler.js

mcollina

lgtm, we can do the rest in issue.

@Uzlopak can you take another look?

Co-authored-by: Aras Abbasi <[email protected]>

lib/handler/cache-handler.js

Co-authored-by: Aras Abbasi <[email protected]>

lib/util/cache.js

Uzlopak

I dont see any obvious code smell any more. :)

LGTM

Uzlopak · 2024-10-14T15:33:34Z

Unrelated Test is flaky. see #3731

ronag requested changes Sep 7, 2024

View reviewed changes

metcoder95 reviewed Sep 8, 2024

View reviewed changes

ronag reviewed Sep 10, 2024

View reviewed changes

lib/handler/cache-handler.js Outdated Show resolved Hide resolved

metcoder95 reviewed Sep 10, 2024

View reviewed changes

lib/cache/lru-cache-store.js Outdated Show resolved Hide resolved

lib/handler/cache-handler.js Outdated Show resolved Hide resolved

lib/handler/cache-handler.js Show resolved Hide resolved

lib/handler/cache-handler.js Outdated Show resolved Hide resolved

ronag mentioned this pull request Sep 11, 2024

feat(cache): use vary headers to compare cached response with request headers nxtedition/nxt-undici#3

Closed

ronag reviewed Sep 11, 2024

View reviewed changes

lib/cache/lru-cache-store.js Outdated Show resolved Hide resolved

ronag reviewed Sep 11, 2024

View reviewed changes

lib/handler/cache-handler.js Outdated Show resolved Hide resolved

flakey5 force-pushed the flakey5/3231 branch 2 times, most recently from e07e3ec to 938fa7a Compare September 12, 2024 03:58

flakey5 force-pushed the flakey5/3231 branch from 938fa7a to f69a8b2 Compare September 14, 2024 05:14

fixup! feat: http caching

57c0f45

Signed-off-by: flakey5 <[email protected]>

flakey5 marked this pull request as ready for review September 14, 2024 05:29

mcollina requested changes Sep 14, 2024

View reviewed changes

types/interceptors.d.ts Show resolved Hide resolved

test/cache-interceptor/cache-stores.js Outdated Show resolved Hide resolved

test/cache-interceptor/interceptor.js Outdated Show resolved Hide resolved

fixup! fixup! feat: http caching

c88fb25

Signed-off-by: flakey5 <[email protected]>

ronag reviewed Sep 15, 2024

View reviewed changes

lib/cache/memory-cache-store.js Show resolved Hide resolved

lib/handler/cache-handler.js Outdated Show resolved Hide resolved

flakey5 and others added 2 commits September 15, 2024 10:55

fixup! fixup! fixup! feat: http caching

0a442b2

Signed-off-by: flakey5 <[email protected]>

Update lib/handler/cache-handler.js

ae6edca

Co-authored-by: Robert Nagy <[email protected]>

metcoder95 reviewed Sep 16, 2024

View reviewed changes

flakey5 and others added 3 commits September 16, 2024 11:21

Apply suggestions from code review

26b2227

Co-authored-by: Carlos Fuentes <[email protected]>

fixup! fixup! fixup! fixup! feat: http caching

81cb021

Signed-off-by: flakey5 <[email protected]>

fixup! fixup! fixup! fixup! fixup! feat: http caching

6bff376

Signed-off-by: flakey5 <[email protected]>