From 6c519d4b457bf1fbeeccb3ed0d01adbd40952e65 Mon Sep 17 00:00:00 2001 From: JJ Kasper Date: Wed, 15 May 2024 14:58:43 -0700 Subject: [PATCH] Separate app router cache entries from pages and use buffer (#65808) Continues https://github.com/vercel/next.js/pull/65664/files and fixes use stringifying the RSC payload un-necessarily which is causing encoding issues. --- .../next/src/server/app-render/app-render.tsx | 2 +- packages/next/src/server/base-server.ts | 73 +++++++++++++---- .../lib/incremental-cache/fetch-cache.ts | 5 +- .../incremental-cache/file-system-cache.ts | 80 +++++++++++-------- packages/next/src/server/render-result.ts | 33 +++++++- .../next/src/server/response-cache/types.ts | 30 ++++++- .../next/src/server/response-cache/utils.ts | 24 +++++- .../stream-utils/node-web-streams-helper.ts | 22 +++++ 8 files changed, 209 insertions(+), 60 deletions(-) diff --git a/packages/next/src/server/app-render/app-render.tsx b/packages/next/src/server/app-render/app-render.tsx index 18f7d5c9f028e..2ef12e60e16ba 100644 --- a/packages/next/src/server/app-render/app-render.tsx +++ b/packages/next/src/server/app-render/app-render.tsx @@ -395,7 +395,7 @@ function createFlightDataResolver(ctx: AppRenderContext) { // Generate the flight data and as soon as it can, convert it into a string. const promise = generateFlight(ctx) .then(async (result) => ({ - flightData: await result.toUnchunkedString(true), + flightData: await result.toUnchunkedBuffer(true), })) // Otherwise if it errored, return the error. .catch((err) => ({ err })) diff --git a/packages/next/src/server/base-server.ts b/packages/next/src/server/base-server.ts index b13f9f27f061c..521fc48ffd817 100644 --- a/packages/next/src/server/base-server.ts +++ b/packages/next/src/server/base-server.ts @@ -13,6 +13,8 @@ import type { ParsedUrlQuery } from 'querystring' import type { RenderOptsPartial as PagesRenderOptsPartial } from './render' import type { RenderOptsPartial as AppRenderOptsPartial } from './app-render/types' import type { + CachedAppPageValue, + CachedPageValue, ResponseCacheBase, ResponseCacheEntry, ResponseGenerator, @@ -2528,16 +2530,29 @@ export default abstract class Server< return null } + if (isAppPath) { + return { + value: { + kind: 'APP_PAGE', + html: result, + headers, + rscData: metadata.flightData, + postponed: metadata.postponed, + status: res.statusCode, + } satisfies CachedAppPageValue, + revalidate: metadata.revalidate, + } + } + // We now have a valid HTML result that we can return to the user. return { value: { kind: 'PAGE', html: result, - pageData: metadata.pageData ?? metadata.flightData, - postponed: metadata.postponed, + pageData: metadata.pageData, headers, - status: isAppPath ? res.statusCode : undefined, - }, + status: res.statusCode, + } satisfies CachedPageValue, revalidate: metadata.revalidate, } } @@ -2650,7 +2665,6 @@ export default abstract class Server< value: { kind: 'PAGE', html: RenderResult.fromStatic(html), - postponed: undefined, status: undefined, headers: undefined, pageData: {}, @@ -2719,7 +2733,7 @@ export default abstract class Server< } const didPostpone = - cacheEntry.value?.kind === 'PAGE' && + cacheEntry.value?.kind === 'APP_PAGE' && typeof cacheEntry.value.postponed === 'string' if ( @@ -2886,7 +2900,11 @@ export default abstract class Server< } else if (isAppPath) { // If the request has a postponed state and it's a resume request we // should error. - if (cachedData.postponed && minimalPostponed) { + if ( + cachedData.kind === 'APP_PAGE' && + cachedData.postponed && + minimalPostponed + ) { throw new Error( 'Invariant: postponed state should not be present on a resume request' ) @@ -2934,7 +2952,11 @@ export default abstract class Server< } // Mark that the request did postpone if this is a data request. - if (cachedData.postponed && isRSCRequest) { + if ( + cachedData.kind === 'APP_PAGE' && + cachedData.postponed && + isRSCRequest + ) { res.setHeader(NEXT_DID_POSTPONE_HEADER, '1') } @@ -2945,8 +2967,15 @@ export default abstract class Server< if (isDataReq && !isPreviewMode) { // If this is a dynamic RSC request, then stream the response. if (isDynamicRSCRequest) { - if (cachedData.pageData) { - throw new Error('Invariant: Expected pageData to be undefined') + if (cachedData.kind !== 'APP_PAGE') { + console.error({ url: req.url, pathname }, cachedData) + throw new Error( + `Invariant: expected cache data kind of APP_PAGE got ${cachedData.kind}` + ) + } + + if (cachedData.rscData) { + throw new Error('Invariant: Expected rscData to be undefined') } if (cachedData.postponed) { @@ -2965,9 +2994,15 @@ export default abstract class Server< } } - if (typeof cachedData.pageData !== 'string') { + if (cachedData.kind !== 'APP_PAGE') { + throw new Error( + `Invariant: expected cached data to be APP_PAGE got ${cachedData.kind}` + ) + } + + if (!Buffer.isBuffer(cachedData.rscData)) { throw new Error( - `Invariant: expected pageData to be a string, got ${typeof cachedData.pageData}` + `Invariant: expected rscData to be a Buffer, got ${typeof cachedData.rscData}` ) } @@ -2975,7 +3010,7 @@ export default abstract class Server< // data. return { type: 'rsc', - body: RenderResult.fromStatic(cachedData.pageData), + body: RenderResult.fromStatic(cachedData.rscData), revalidate: cacheEntry.revalidate, } } @@ -2986,7 +3021,10 @@ export default abstract class Server< // If there's no postponed state, we should just serve the HTML. This // should also be the case for a resume request because it's completed // as a server render (rather than a static render). - if (!cachedData.postponed || this.minimalMode) { + if ( + !(cachedData.kind === 'APP_PAGE' && cachedData.postponed) || + this.minimalMode + ) { return { type: 'html', body, @@ -3015,7 +3053,7 @@ export default abstract class Server< throw new Error('Invariant: expected a result to be returned') } - if (result.value?.kind !== 'PAGE') { + if (result.value?.kind !== 'APP_PAGE') { throw new Error( `Invariant: expected a page response, got ${result.value?.kind}` ) @@ -3041,6 +3079,11 @@ export default abstract class Server< revalidate: 0, } } else if (isDataReq) { + if (cachedData.kind !== 'PAGE') { + throw new Error( + `Invariant: expected cached data to be PAGE got ${cachedData.kind}` + ) + } return { type: 'json', body: RenderResult.fromStatic(JSON.stringify(cachedData.pageData)), diff --git a/packages/next/src/server/lib/incremental-cache/fetch-cache.ts b/packages/next/src/server/lib/incremental-cache/fetch-cache.ts index 6ecb7048eda90..55d77c0f228fc 100644 --- a/packages/next/src/server/lib/incremental-cache/fetch-cache.ts +++ b/packages/next/src/server/lib/incremental-cache/fetch-cache.ts @@ -109,7 +109,10 @@ export default class FetchCache implements CacheHandler { } // rough estimate of size of cache value return ( - value.html.length + (JSON.stringify(value.pageData)?.length || 0) + value.html.length + + (JSON.stringify( + value.kind === 'APP_PAGE' ? value.rscData : value.pageData + )?.length || 0) ) }, }) diff --git a/packages/next/src/server/lib/incremental-cache/file-system-cache.ts b/packages/next/src/server/lib/incremental-cache/file-system-cache.ts index cca5da3030263..32119dd6085e8 100644 --- a/packages/next/src/server/lib/incremental-cache/file-system-cache.ts +++ b/packages/next/src/server/lib/incremental-cache/file-system-cache.ts @@ -77,7 +77,10 @@ export default class FileSystemCache implements CacheHandler { } // rough estimate of size of cache value return ( - value.html.length + (JSON.stringify(value.pageData)?.length || 0) + value.html.length + + (JSON.stringify( + value.kind === 'APP_PAGE' ? value.rscData : value.pageData + )?.length || 0) ) }, }) @@ -229,23 +232,6 @@ export default class FileSystemCache implements CacheHandler { } } } else { - const pageData = isAppPath - ? await this.fs.readFile( - this.getFilePath( - `${key}${ - this.isAppPPREnabled ? RSC_PREFETCH_SUFFIX : RSC_SUFFIX - }`, - 'app' - ), - 'utf8' - ) - : JSON.parse( - await this.fs.readFile( - this.getFilePath(`${key}${NEXT_DATA_SUFFIX}`, 'pages'), - 'utf8' - ) - ) - let meta: RouteMetadata | undefined if (isAppPath) { @@ -259,16 +245,44 @@ export default class FileSystemCache implements CacheHandler { } catch {} } - data = { - lastModified: mtime.getTime(), - value: { - kind: 'PAGE', - html: fileData, - pageData, - postponed: meta?.postponed, - headers: meta?.headers, - status: meta?.status, - }, + if (isAppPath) { + const rscData = await this.fs.readFile( + this.getFilePath( + `${key}${ + this.isAppPPREnabled ? RSC_PREFETCH_SUFFIX : RSC_SUFFIX + }`, + 'app' + ) + ) + data = { + lastModified: mtime.getTime(), + value: { + kind: 'APP_PAGE', + html: fileData, + rscData, + postponed: meta?.postponed, + headers: meta?.headers, + status: meta?.status, + }, + } + } else { + const pageData = JSON.parse( + await this.fs.readFile( + this.getFilePath(`${key}${NEXT_DATA_SUFFIX}`, 'pages'), + 'utf8' + ) + ) + + data = { + lastModified: mtime.getTime(), + value: { + kind: 'PAGE', + html: fileData, + pageData, + headers: meta?.headers, + status: meta?.status, + }, + } } } @@ -280,7 +294,7 @@ export default class FileSystemCache implements CacheHandler { } } - if (data?.value?.kind === 'PAGE') { + if (data?.value?.kind === 'APP_PAGE' || data?.value?.kind === 'PAGE') { let cacheTags: undefined | string[] const tagsHeader = data.value.headers?.[NEXT_CACHE_TAGS_HEADER] @@ -364,8 +378,8 @@ export default class FileSystemCache implements CacheHandler { return } - if (data?.kind === 'PAGE') { - const isAppPath = typeof data.pageData === 'string' + if (data?.kind === 'PAGE' || data?.kind === 'APP_PAGE') { + const isAppPath = 'rscData' in data const htmlPath = this.getFilePath( `${key}.html`, isAppPath ? 'app' : 'pages' @@ -384,14 +398,14 @@ export default class FileSystemCache implements CacheHandler { }`, isAppPath ? 'app' : 'pages' ), - isAppPath ? data.pageData : JSON.stringify(data.pageData) + isAppPath ? data.rscData : JSON.stringify(data.pageData) ) if (data.headers || data.status) { const meta: RouteMetadata = { headers: data.headers, status: data.status, - postponed: data.postponed, + postponed: isAppPath ? data.postponed : undefined, } await this.fs.writeFile( diff --git a/packages/next/src/server/render-result.ts b/packages/next/src/server/render-result.ts index 57bd89695dbfb..448a0431795e6 100644 --- a/packages/next/src/server/render-result.ts +++ b/packages/next/src/server/render-result.ts @@ -4,7 +4,9 @@ import type { FetchMetrics } from './base-http' import { chainStreams, + streamFromBuffer, streamFromString, + streamToBuffer, streamToString, } from './stream-utils/node-web-streams-helper' import { isAbortError, pipeToNodeResponse } from './pipe-readable' @@ -12,7 +14,7 @@ import { isAbortError, pipeToNodeResponse } from './pipe-readable' type ContentTypeOption = string | undefined export type AppPageRenderResultMetadata = { - flightData?: string + flightData?: Buffer revalidate?: Revalidate staticBailoutInfo?: { stack?: string @@ -50,6 +52,7 @@ export type RenderResultResponse = | ReadableStream[] | ReadableStream | string + | Buffer | null export type RenderResultOptions< @@ -89,7 +92,7 @@ export default class RenderResult< * @param value the static response value * @returns a new RenderResult instance */ - public static fromStatic(value: string) { + public static fromStatic(value: string | Buffer) { return new RenderResult(value, { metadata: {} }) } @@ -125,6 +128,26 @@ export default class RenderResult< return typeof this.response !== 'string' } + public toUnchunkedBuffer(stream?: false): Buffer + public toUnchunkedBuffer(stream: true): Promise + public toUnchunkedBuffer(stream = false): Promise | Buffer { + if (this.response === null) { + throw new Error('Invariant: null responses cannot be unchunked') + } + + if (typeof this.response !== 'string') { + if (!stream) { + throw new Error( + 'Invariant: dynamic responses cannot be unchunked. This is a bug in Next.js' + ) + } + + return streamToBuffer(this.readable) + } + + return Buffer.from(this.response) + } + /** * Returns the response if it is a string. If the page was dynamic, this will * return a promise if the `stream` option is true, or it will throw an error. @@ -164,6 +187,10 @@ export default class RenderResult< throw new Error('Invariant: static responses cannot be streamed') } + if (Buffer.isBuffer(this.response)) { + return streamFromBuffer(this.response) + } + // If the response is an array of streams, then chain them together. if (Array.isArray(this.response)) { return chainStreams(...this.response) @@ -191,6 +218,8 @@ export default class RenderResult< responses = [streamFromString(this.response)] } else if (Array.isArray(this.response)) { responses = this.response + } else if (Buffer.isBuffer(this.response)) { + responses = [streamFromBuffer(this.response)] } else { responses = [this.response] } diff --git a/packages/next/src/server/response-cache/types.ts b/packages/next/src/server/response-cache/types.ts index 4f098edf9932c..e8492bb3f4a0e 100644 --- a/packages/next/src/server/response-cache/types.ts +++ b/packages/next/src/server/response-cache/types.ts @@ -40,12 +40,22 @@ export interface CachedRedirectValue { props: Object } -interface CachedPageValue { - kind: 'PAGE' +export interface CachedAppPageValue { + kind: 'APP_PAGE' // this needs to be a RenderResult so since renderResponse // expects that type instead of a string html: RenderResult + rscData: Buffer | undefined + status: number | undefined postponed: string | undefined + headers: OutgoingHttpHeaders | undefined +} + +export interface CachedPageValue { + kind: 'PAGE' + // this needs to be a RenderResult so since renderResponse + // expects that type instead of a string + html: RenderResult pageData: Object status: number | undefined headers: OutgoingHttpHeaders | undefined @@ -69,13 +79,23 @@ export interface CachedImageValue { isStale?: boolean } -interface IncrementalCachedPageValue { +export interface IncrementalCachedAppPageValue { + kind: 'APP_PAGE' + // this needs to be a string since the cache expects to store + // the string value + html: string + rscData: Buffer | undefined + headers: OutgoingHttpHeaders | undefined + postponed: string | undefined + status: number | undefined +} + +export interface IncrementalCachedPageValue { kind: 'PAGE' // this needs to be a string since the cache expects to store // the string value html: string pageData: Object - postponed: string | undefined headers: OutgoingHttpHeaders | undefined status: number | undefined } @@ -92,6 +112,7 @@ export type IncrementalCacheEntry = { export type IncrementalCacheValue = | CachedRedirectValue | IncrementalCachedPageValue + | IncrementalCachedAppPageValue | CachedImageValue | CachedFetchValue | CachedRouteValue @@ -99,6 +120,7 @@ export type IncrementalCacheValue = export type ResponseCacheValue = | CachedRedirectValue | CachedPageValue + | CachedAppPageValue | CachedImageValue | CachedRouteValue diff --git a/packages/next/src/server/response-cache/utils.ts b/packages/next/src/server/response-cache/utils.ts index 174d6fa619a4a..18ec64c1dd052 100644 --- a/packages/next/src/server/response-cache/utils.ts +++ b/packages/next/src/server/response-cache/utils.ts @@ -12,12 +12,20 @@ export async function fromResponseCacheEntry( ? { kind: 'PAGE', html: await cacheEntry.value.html.toUnchunkedString(true), - postponed: cacheEntry.value.postponed, pageData: cacheEntry.value.pageData, headers: cacheEntry.value.headers, status: cacheEntry.value.status, } - : cacheEntry.value, + : cacheEntry.value?.kind === 'APP_PAGE' + ? { + kind: 'APP_PAGE', + html: await cacheEntry.value.html.toUnchunkedString(true), + postponed: cacheEntry.value.postponed, + rscData: cacheEntry.value.rscData, + headers: cacheEntry.value.headers, + status: cacheEntry.value.status, + } + : cacheEntry.value, } } @@ -42,10 +50,18 @@ export async function toResponseCacheEntry( kind: 'PAGE', html: RenderResult.fromStatic(response.value.html), pageData: response.value.pageData, - postponed: response.value.postponed, headers: response.value.headers, status: response.value.status, } - : response.value, + : response.value?.kind === 'APP_PAGE' + ? { + kind: 'APP_PAGE', + html: RenderResult.fromStatic(response.value.html), + rscData: response.value.rscData, + headers: response.value.headers, + status: response.value.status, + postponed: response.value.postponed, + } + : response.value, } } diff --git a/packages/next/src/server/stream-utils/node-web-streams-helper.ts b/packages/next/src/server/stream-utils/node-web-streams-helper.ts index a3d4ae8b924ee..8b051257c5868 100644 --- a/packages/next/src/server/stream-utils/node-web-streams-helper.ts +++ b/packages/next/src/server/stream-utils/node-web-streams-helper.ts @@ -73,6 +73,28 @@ export function streamFromString(str: string): ReadableStream { }) } +export function streamFromBuffer(chunk: Buffer): ReadableStream { + return new ReadableStream({ + start(controller) { + controller.enqueue(chunk) + controller.close() + }, + }) +} + +export async function streamToBuffer( + stream: ReadableStream +): Promise { + const buffers: Buffer[] = [] + + // @ts-expect-error TypeScript gets this wrong (https://nodejs.org/api/webstreams.html#async-iteration) + for await (const chunk of stream) { + buffers.push(chunk) + } + + return Buffer.concat(buffers) +} + export async function streamToString( stream: ReadableStream ): Promise {