lib: more cache fixes by flakey5 · Pull Request #3816 · nodejs/undici

flakey5 · 2024-11-08T16:33:36Z

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
- Reasoning: The cache handler should not invalidate all requests to an origin #3806
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 <73616808+flakey5@users.noreply.github.com>

docs/docs/api/CacheStore.md

lib/handler/cache-handler.js

ronag · 2024-11-08T20:38:59Z

lib/interceptor/cache.js

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


Can stream be nully? When?

On HEAD requests

the asserts already catch it

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

lib/interceptor/cache.js

lib/util/cache.js

lib/core/util.js

Co-authored-by: Robert Nagy <ronagy@icloud.com>

DTrombett · 2024-11-09T17:35:53Z

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

undici/lib/handler/cache-handler.js

Lines 236 to 243 in 9f467e7

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

flakey5 · 2024-11-09T18:02:36Z

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 <73616808+flakey5@users.noreply.github.com>

mcollina

lgtm

lib: more cache fixes

868af1c

Closes nodejs#3806 Signed-off-by: flakey5 <73616808+flakey5@users.noreply.github.com>