-
Notifications
You must be signed in to change notification settings - Fork 86
fix: handle case of zero-length cacheable route handler responses #2819
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
Changes from 3 commits
c5037d7
05e328c
006ce40
f1ab628
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,44 +25,71 @@ export function setInMemoryCacheMaxSizeFromNextConfig(size: unknown) { | |
} | ||
} | ||
|
||
const estimateBlobSize = (valueToStore: BlobType | null | Promise<unknown>): number => { | ||
type PositiveNumber = number & { __positive: true } | ||
const isPositiveNumber = (value: unknown): value is PositiveNumber => { | ||
return typeof value === 'number' && value > 0 | ||
} | ||
|
||
const BASE_BLOB_SIZE = 25 as PositiveNumber | ||
|
||
const estimateBlobKnownTypeSize = ( | ||
valueToStore: BlobType | null | Promise<unknown>, | ||
): number | undefined => { | ||
// very approximate size calculation to avoid expensive exact size calculation | ||
// inspired by https://github.com/vercel/next.js/blob/ed10f7ed0246fcc763194197eb9beebcbd063162/packages/next/src/server/lib/incremental-cache/file-system-cache.ts#L60-L79 | ||
if (valueToStore === null || isPromise(valueToStore) || isTagManifest(valueToStore)) { | ||
return 25 | ||
return BASE_BLOB_SIZE | ||
} | ||
if (isHtmlBlob(valueToStore)) { | ||
return valueToStore.html.length | ||
return BASE_BLOB_SIZE + valueToStore.html.length | ||
} | ||
|
||
if (valueToStore.value?.kind === 'FETCH') { | ||
return BASE_BLOB_SIZE + valueToStore.value.data.body.length | ||
} | ||
if (valueToStore.value?.kind === 'APP_PAGE') { | ||
return ( | ||
BASE_BLOB_SIZE + valueToStore.value.html.length + (valueToStore.value.rscData?.length ?? 0) | ||
) | ||
} | ||
if (valueToStore.value?.kind === 'PAGE' || valueToStore.value?.kind === 'PAGES') { | ||
return ( | ||
BASE_BLOB_SIZE + | ||
valueToStore.value.html.length + | ||
JSON.stringify(valueToStore.value.pageData).length | ||
) | ||
} | ||
let knownKindFailed = false | ||
if (valueToStore.value?.kind === 'ROUTE' || valueToStore.value?.kind === 'APP_ROUTE') { | ||
return BASE_BLOB_SIZE + valueToStore.value.body.length | ||
} | ||
} | ||
|
||
const estimateBlobSize = (valueToStore: BlobType | null | Promise<unknown>): PositiveNumber => { | ||
let knownTypeFailed = false | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Technically, you could omit this var now that you're using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll adjust now - it's unlikely this will be done as follow up There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. f1ab628 dropped that extra unneded variable. Also added calculated size to warning just for it to have a bit more information |
||
let estimatedKnownTypeSize: number | undefined | ||
let estimateBlobKnownTypeSizeError: unknown | ||
try { | ||
if (valueToStore.value?.kind === 'FETCH') { | ||
return valueToStore.value.data.body.length | ||
} | ||
if (valueToStore.value?.kind === 'APP_PAGE') { | ||
return valueToStore.value.html.length + (valueToStore.value.rscData?.length ?? 0) | ||
} | ||
if (valueToStore.value?.kind === 'PAGE' || valueToStore.value?.kind === 'PAGES') { | ||
return valueToStore.value.html.length + JSON.stringify(valueToStore.value.pageData).length | ||
} | ||
if (valueToStore.value?.kind === 'ROUTE' || valueToStore.value?.kind === 'APP_ROUTE') { | ||
return valueToStore.value.body.length | ||
estimatedKnownTypeSize = estimateBlobKnownTypeSize(valueToStore) | ||
if (isPositiveNumber(estimatedKnownTypeSize)) { | ||
return estimatedKnownTypeSize | ||
} | ||
} catch { | ||
// size calculation rely on the shape of the value, so if it's not what we expect, we fallback to JSON.stringify | ||
knownKindFailed = true | ||
} catch (error) { | ||
knownTypeFailed = true | ||
estimateBlobKnownTypeSizeError = error | ||
} | ||
|
||
// fallback for not known kinds or known kinds that did fail to calculate size | ||
// fallback for not known kinds or known kinds that did fail to calculate positive size | ||
// we should also monitor cases when fallback is used because it's not the most efficient way to calculate/estimate size | ||
// and might indicate need to make adjustments or additions to the size calculation | ||
recordWarning( | ||
new Error( | ||
`Blob size calculation did fallback to JSON.stringify. Kind: KnownKindFailed: ${knownKindFailed}, ${valueToStore.value?.kind ?? 'undefined'}`, | ||
`Blob size calculation did fallback to JSON.stringify. KnownTypeFailed: ${knownTypeFailed}, EstimatedKnownTypeSize: ${estimatedKnownTypeSize}, ValueToStore: ${JSON.stringify(valueToStore)}`, | ||
estimateBlobKnownTypeSizeError ? { cause: estimateBlobKnownTypeSizeError } : undefined, | ||
), | ||
) | ||
|
||
return JSON.stringify(valueToStore).length | ||
const calculatedSize = JSON.stringify(valueToStore).length | ||
return isPositiveNumber(calculatedSize) ? calculatedSize : BASE_BLOB_SIZE | ||
} | ||
|
||
function getInMemoryLRUCache() { | ||
|
@@ -98,12 +125,26 @@ export const getRequestScopedInMemoryCache = (): RequestScopedInMemoryCache => { | |
return { | ||
get(key) { | ||
if (!requestContext) return | ||
const value = inMemoryLRUCache?.get(`${requestContext.requestID}:${key}`) | ||
return value === NullValue ? null : value | ||
try { | ||
const value = inMemoryLRUCache?.get(`${requestContext.requestID}:${key}`) | ||
return value === NullValue ? null : value | ||
} catch (error) { | ||
// using in-memory store is perf optimization not requirement | ||
// trying to use optimization should NOT cause crashes | ||
// so we just record warning and return undefined | ||
recordWarning(new Error('Failed to get value from memory cache', { cause: error })) | ||
} | ||
}, | ||
set(key, value) { | ||
if (!requestContext) return | ||
inMemoryLRUCache?.set(`${requestContext?.requestID}:${key}`, value ?? NullValue) | ||
try { | ||
inMemoryLRUCache?.set(`${requestContext?.requestID}:${key}`, value ?? NullValue) | ||
} catch (error) { | ||
// using in-memory store is perf optimization not requirement | ||
// trying to use optimization should NOT cause crashes | ||
// so we just record warning and return undefined | ||
recordWarning(new Error('Failed to store value in memory cache', { cause: error })) | ||
} | ||
}, | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
export async function GET() { | ||
return new Response('') | ||
} | ||
|
||
export const dynamic = 'force-static' |
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 in particular is the fix for added test case.
But using it as example, I did add more adjustments:
estimateBlobSize
returns "PositiveNumber
"