-
-
Notifications
You must be signed in to change notification settings - Fork 671
lib: more cache fixes #3816
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
Conversation
Closes nodejs#3806 Signed-off-by: flakey5 <[email protected]>
| assert(!stream || !stream.destroyed, 'stream should not be destroyed') | ||
| assert(!stream || !stream.readableDidRead, 'stream should not be readableDidRead') | ||
| try { | ||
| stream |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On HEAD requests
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wdym?
Co-authored-by: Robert Nagy <[email protected]>
|
Asking here as it seems related, but why is the undici/lib/handler/cache-handler.js Lines 236 to 243 in 9f467e7
|
Will fix, thanks! |
Signed-off-by: flakey5 <[email protected]>
bb502d9 to
0856e3e
Compare
Signed-off-by: flakey5 <[email protected]>
Signed-off-by: flakey5 <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Closes #3806
CacheStoreReadableandCacheStoreWritable, replaces w/ justReadableandWritableCacheStoreValuetype toCachedResponseCacheStore#createReadStreamfunction (see one below on reasoning)CacheStore#getfunction that acts similarly tocreateReadStreamGetResultwhich separates theCacheResponseandReadablefor the bodyMemoryCacheStorereadLockandwriteLockcondensed into justlockedthat only stops value from being readCacheKeytype that we pass into the cache handler, cache storesisFulloptionaldeleteByOriginwithdeleteByKeyCachedValuetypebodystream and the properties inCachedResponsecc @mcollina @ronag