Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lib: more cache fixes #3816

Merged
merged 5 commits into from
Nov 13, 2024
Merged

Conversation

flakey5
Copy link
Member

@flakey5 flakey5 commented Nov 8, 2024

Closes #3806

  • Removes CacheStoreReadable and CacheStoreWritable, replaces w/ just Readable and Writable
    • Reasoning: api simplicity
  • Renames CacheStoreValue type to CachedResponse
  • Removes CacheStore#createReadStream function (see one below on reasoning)
  • Adds CacheStore#get function that acts similarly to createReadStream
    • Reasoning: api simplicity & niceness. Returns a GetResult which separates the CacheResponse and Readable for the body
  • Simplifies MemoryCacheStore
    • Inline readable & writables
    • readLock and writeLock condensed into just locked that only stops value from being read
  • Introduces CacheKey type that we pass into the cache handler, cache stores
    • Replaces us passing in the full request options
    • Reasoning: cache stores don't need the full thing, guides them to be more spec-compliant
  • Various internal type fixes
  • Fixes regarding a header being present multiple times
    • Stripping headers would result in them being re-encoded in the wrong format
    • Cache control & vary header parsings
  • Makes isFull optional
    • Reasoning: some cache stores can't get full / don't have a max. This only exists as a very small optimization
  • Removes caching of trailers
    • Reasoning: simplicity sakes, ideally this will be added back later
  • Replace deleteByOrigin with deleteByKey
  • Adds CachedValue type
    • Separates the body stream and the properties in CachedResponse
    • Allows the stream to be undefined for HEAD requests since there's no body

cc @mcollina @ronag

Closes nodejs#3806

Signed-off-by: flakey5 <[email protected]>
docs/docs/api/CacheStore.md Outdated Show resolved Hide resolved
lib/handler/cache-handler.js Outdated Show resolved Hide resolved
lib/handler/cache-handler.js Outdated Show resolved Hide resolved
assert(!stream.readableDidRead, 'stream should not be readableDidRead')

assert(!stream || !stream.destroyed, 'stream should not be destroyed')
assert(!stream || !stream.readableDidRead, 'stream should not be readableDidRead')
try {
stream
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can stream be nully? When?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On HEAD requests

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the asserts already catch it

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is broken though like this... onConnect cannot abort the request

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wdym?

lib/interceptor/cache.js Outdated Show resolved Hide resolved
lib/interceptor/cache.js Outdated Show resolved Hide resolved
lib/util/cache.js Outdated Show resolved Hide resolved
lib/core/util.js Outdated Show resolved Hide resolved
@DTrombett
Copy link
Contributor

DTrombett commented Nov 9, 2024

Asking here as it seems related, but why is the public directive required? Shouldn't it be required only with authenticated requests?

if (
!cacheControlDirectives.public ||
cacheControlDirectives.private === true ||
cacheControlDirectives['no-cache'] === true ||
cacheControlDirectives['no-store']
) {
return false
}

@flakey5
Copy link
Member Author

flakey5 commented Nov 9, 2024

Asking here as it seems related, but why is the public directive required? Shouldn't it be required only with authenticated requests?

Will fix, thanks!

Signed-off-by: flakey5 <[email protected]>
@flakey5 flakey5 force-pushed the flakey5/20241106/cache-updates branch from bb502d9 to 0856e3e Compare November 12, 2024 20:23
@flakey5 flakey5 marked this pull request as ready for review November 12, 2024 20:39
Signed-off-by: flakey5 <[email protected]>
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@ronag ronag merged commit 51836bb into nodejs:main Nov 13, 2024
33 of 36 checks passed
flakey5 added a commit to flakey5/undici that referenced this pull request Nov 14, 2024
@github-actions github-actions bot mentioned this pull request Dec 3, 2024
This was referenced Dec 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The cache handler should not invalidate all requests to an origin
5 participants