From 3116806738c0c44a8bb61de01dc357cd31fbd0c8 Mon Sep 17 00:00:00 2001 From: Josh Story Date: Thu, 24 Oct 2024 11:39:21 -0700 Subject: [PATCH] [dynamicIO] wait for cache signal before validating wait for caches to fill in dev before performing validation --- .../next/src/server/app-render/app-render.tsx | 119 ++++++++---------- packages/next/src/server/base-server.ts | 23 ++-- .../src/server/lib/prefetch-cache-scopes.ts | 11 +- .../src/server/use-cache/use-cache-wrapper.ts | 40 +++++- .../app/api/data.ts | 9 ++ .../app/cached/page.tsx | 42 +++++++ .../dynamic-io-dev-cache-scope/app/layout.tsx | 9 ++ .../app/uncached/page.tsx | 33 +++++ .../dynamic-io-dev-cache-scope.test.ts | 98 +++++++++++++++ .../dynamic-io-dev-cache-scope/next.config.js | 10 ++ .../dynamic-io/dynamic-io.draft-mode.test.ts | 2 + 11 files changed, 314 insertions(+), 82 deletions(-) create mode 100644 test/development/app-dir/dynamic-io-dev-cache-scope/app/api/data.ts create mode 100644 test/development/app-dir/dynamic-io-dev-cache-scope/app/cached/page.tsx create mode 100644 test/development/app-dir/dynamic-io-dev-cache-scope/app/layout.tsx create mode 100644 test/development/app-dir/dynamic-io-dev-cache-scope/app/uncached/page.tsx create mode 100644 test/development/app-dir/dynamic-io-dev-cache-scope/dynamic-io-dev-cache-scope.test.ts create mode 100644 test/development/app-dir/dynamic-io-dev-cache-scope/next.config.js diff --git a/packages/next/src/server/app-render/app-render.tsx b/packages/next/src/server/app-render/app-render.tsx index 23abfe16730bd7..b6d2a1ffafa8af 100644 --- a/packages/next/src/server/app-render/app-render.tsx +++ b/packages/next/src/server/app-render/app-render.tsx @@ -1942,6 +1942,7 @@ async function spawnDynamicValidationInDev( ): Promise { const { componentMod: ComponentMod } = ctx + const cacheSignal = new CacheSignal() const firstAttemptServerController = new AbortController() let serverDynamicTracking = createDynamicTrackingState(false) @@ -1950,7 +1951,7 @@ async function spawnDynamicValidationInDev( phase: 'render', implicitTags: [], renderSignal: firstAttemptServerController.signal, - cacheSignal: null, + cacheSignal, // During the prospective render we don't want to synchronously abort on dynamic access // because it could prevent us from discovering all caches in siblings. So we omit the controller // from the prerender store this time. @@ -1958,12 +1959,12 @@ async function spawnDynamicValidationInDev( // With PPR during Prerender we don't need to track individual dynamic reasons // because we will always do a final render after caches have filled and we // will track it again there - dynamicTracking: serverDynamicTracking, + dynamicTracking: null, revalidate: INFINITE_CACHE, expire: INFINITE_CACHE, stale: INFINITE_CACHE, tags: [], - // Dev only property that allows certain logs to be supressed + // Dev only property that allows certain logs to be suppressed validating: true, } @@ -1977,80 +1978,70 @@ async function spawnDynamicValidationInDev( let reactServerStream = await workUnitAsyncStorage.run( firstAttemptServerPrerenderStore, + ComponentMod.renderToReadableStream, + firstAttemptRSCPayload, + clientReferenceManifest.clientModules, + { + signal: firstAttemptServerController.signal, + onError: () => {}, + } + ) + + await cacheSignal.cacheReady() + firstAttemptServerController.abort() + + const secondAttemptServerController = new AbortController() + serverDynamicTracking = createDynamicTrackingState(false) + + const secondAttemptServerPrerenderStore: PrerenderStore = { + type: 'prerender', + phase: 'render', + implicitTags: [], + renderSignal: secondAttemptServerController.signal, + cacheSignal: null, + // During the prospective render we don't want to synchronously abort on dynamic access + // because it could prevent us from discovering all caches in siblings. So we omit the controller + // from the prerender store this time. + controller: secondAttemptServerController, + // With PPR during Prerender we don't need to track individual dynamic reasons + // because we will always do a final render after caches have filled and we + // will track it again there + dynamicTracking: serverDynamicTracking, + revalidate: INFINITE_CACHE, + expire: INFINITE_CACHE, + stale: INFINITE_CACHE, + tags: [], + // Dev only property that allows certain logs to be suppressed + validating: true, + } + + const secondAttemptRSCPayload = await workUnitAsyncStorage.run( + secondAttemptServerPrerenderStore, + getRSCPayload, + tree, + ctx, + isNotFound + ) + + reactServerStream = await workUnitAsyncStorage.run( + secondAttemptServerPrerenderStore, scheduleInSequentialTasks, () => { const stream = ComponentMod.renderToReadableStream( - firstAttemptRSCPayload, + secondAttemptRSCPayload, clientReferenceManifest.clientModules, { - signal: firstAttemptServerController.signal, + signal: secondAttemptServerController.signal, onError: () => {}, } ) - return asHaltedStream(stream, firstAttemptServerController.signal) + return asHaltedStream(stream, secondAttemptServerController.signal) }, () => { - firstAttemptServerController.abort() + secondAttemptServerController.abort() } ) - if (serverDynamicTracking.syncDynamicErrorWithStack) { - // If we had a sync dynamic error then we need to retry without - reactServerStream.cancel() - - const secondAttemptServerController = new AbortController() - serverDynamicTracking = createDynamicTrackingState(false) - - const secondAttemptServerPrerenderStore: PrerenderStore = { - type: 'prerender', - phase: 'render', - implicitTags: [], - renderSignal: secondAttemptServerController.signal, - cacheSignal: null, - // During the prospective render we don't want to synchronously abort on dynamic access - // because it could prevent us from discovering all caches in siblings. So we omit the controller - // from the prerender store this time. - controller: secondAttemptServerController, - // With PPR during Prerender we don't need to track individual dynamic reasons - // because we will always do a final render after caches have filled and we - // will track it again there - dynamicTracking: serverDynamicTracking, - revalidate: INFINITE_CACHE, - expire: INFINITE_CACHE, - stale: INFINITE_CACHE, - tags: [], - // Dev only property that allows certain logs to be supressed - validating: true, - } - - const secondAttemptRSCPayload = await workUnitAsyncStorage.run( - secondAttemptServerPrerenderStore, - getRSCPayload, - tree, - ctx, - isNotFound - ) - - reactServerStream = await workUnitAsyncStorage.run( - secondAttemptServerPrerenderStore, - scheduleInSequentialTasks, - () => { - const stream = ComponentMod.renderToReadableStream( - secondAttemptRSCPayload, - clientReferenceManifest.clientModules, - { - signal: secondAttemptServerController.signal, - onError: () => {}, - } - ) - return asHaltedStream(stream, secondAttemptServerController.signal) - }, - () => { - secondAttemptServerController.abort() - } - ) - } - const [warmupStream, renderStream] = reactServerStream.tee() await warmFlightResponse(warmupStream, clientReferenceManifest) diff --git a/packages/next/src/server/base-server.ts b/packages/next/src/server/base-server.ts index 3c29b6d3daf604..9281e6344c36ac 100644 --- a/packages/next/src/server/base-server.ts +++ b/packages/next/src/server/base-server.ts @@ -3085,34 +3085,27 @@ export default abstract class Server< if (this.renderOpts.dev) { let cache = this.prefetchCacheScopesDev.get(urlPathname) + if (isServerAction || !cache) { + cache = new Map() + this.prefetchCacheScopesDev.set(urlPathname, cache) + } + // we need to seed the prefetch cache scope in dev // since we did not have a prefetch cache available // and this is not a prefetch request if ( - !cache && !isPrefetchRSCRequest && routeModule?.definition.kind === RouteKind.APP_PAGE && !isServerAction ) { - cache = new Map() - await runWithCacheScope({ cache }, () => originalResponseGenerator({ ...state, isDevWarmup: true }) ) - this.prefetchCacheScopesDev.set(urlPathname, cache) } - if (cache) { - return runWithCacheScope({ cache }, () => - originalResponseGenerator(state) - ).finally(() => { - if (isPrefetchRSCRequest) { - this.prefetchCacheScopesDev.set(urlPathname, cache) - } else { - this.prefetchCacheScopesDev.del(urlPathname) - } - }) - } + return runWithCacheScope({ cache }, () => + originalResponseGenerator(state) + ) } return originalResponseGenerator(state) diff --git a/packages/next/src/server/lib/prefetch-cache-scopes.ts b/packages/next/src/server/lib/prefetch-cache-scopes.ts index cc3617601cc969..d61812f6017501 100644 --- a/packages/next/src/server/lib/prefetch-cache-scopes.ts +++ b/packages/next/src/server/lib/prefetch-cache-scopes.ts @@ -13,7 +13,7 @@ export class PrefetchCacheScopes { private evict() { for (const [key, value] of this.cacheScopes) { - if (value.timestamp < Date.now() - 30_000) { + if (value.timestamp < Date.now() - 5_000) { this.cacheScopes.delete(key) } } @@ -23,7 +23,14 @@ export class PrefetchCacheScopes { // filter _rsc query get(url: string) { setImmediate(() => this.evict()) - return this.cacheScopes.get(url)?.cache + const currentScope = this.cacheScopes.get(url) + if (currentScope) { + if (currentScope.timestamp < Date.now() - 5_000) { + return undefined + } + return currentScope.cache + } + return undefined } set(url: string, cache: CacheScopeStore['cache']) { diff --git a/packages/next/src/server/use-cache/use-cache-wrapper.ts b/packages/next/src/server/use-cache/use-cache-wrapper.ts index b681d8aa4588a9..13f1ddac1885bd 100644 --- a/packages/next/src/server/use-cache/use-cache-wrapper.ts +++ b/packages/next/src/server/use-cache/use-cache-wrapper.ts @@ -516,6 +516,14 @@ export function cache(kind: string, id: string, fn: any) { const cacheScope: undefined | CacheScopeStore = cacheScopeAsyncLocalStorage.getStore() if (cacheScope) { + const cacheSignal = + workUnitStore && workUnitStore.type === 'prerender' + ? workUnitStore.cacheSignal + : null + + if (cacheSignal) { + cacheSignal.beginRead() + } const cachedEntry: undefined | Promise = cacheScope.cache.get(serializedCacheKey) if (cachedEntry !== undefined) { @@ -532,6 +540,9 @@ export function cache(kind: string, id: string, fn: any) { // expire time is under 5 minutes, then we consider this cache entry dynamic // as it's not worth generating static pages for such data. It's better to leave // a PPR hole that can be filled in dynamically with a potentially cached entry. + if (cacheSignal) { + cacheSignal.endRead() + } return makeHangingPromise( workUnitStore.renderSignal, 'dynamic "use cache"' @@ -539,7 +550,34 @@ export function cache(kind: string, id: string, fn: any) { } const [streamA, streamB] = existingEntry.value.tee() existingEntry.value = streamB - stream = streamA + + if (cacheSignal) { + // When we have a cacheSignal we need to block on reading the cache + // entry before ending the read. + const buffer: any[] = [] + const reader = streamA.getReader() + for (let entry; !(entry = await reader.read()).done; ) { + buffer.push(entry.value) + } + + let idx = 0 + stream = new ReadableStream({ + pull(controller) { + if (idx < buffer.length) { + controller.enqueue(buffer[idx++]) + } else { + controller.close() + } + }, + }) + cacheSignal.endRead() + } else { + stream = streamA + } + } else { + if (cacheSignal) { + cacheSignal.endRead() + } } } diff --git a/test/development/app-dir/dynamic-io-dev-cache-scope/app/api/data.ts b/test/development/app-dir/dynamic-io-dev-cache-scope/app/api/data.ts new file mode 100644 index 00000000000000..1b8389d4569523 --- /dev/null +++ b/test/development/app-dir/dynamic-io-dev-cache-scope/app/api/data.ts @@ -0,0 +1,9 @@ +function delay() { + return new Promise((resolve) => { + setTimeout(resolve, 100) + }) +} +export async function fetchData() { + await delay() + return '' + Math.random() +} diff --git a/test/development/app-dir/dynamic-io-dev-cache-scope/app/cached/page.tsx b/test/development/app-dir/dynamic-io-dev-cache-scope/app/cached/page.tsx new file mode 100644 index 00000000000000..4b6b58188aa4ff --- /dev/null +++ b/test/development/app-dir/dynamic-io-dev-cache-scope/app/cached/page.tsx @@ -0,0 +1,42 @@ +import { + revalidateTag, + unstable_cacheLife as cacheLife, + unstable_cacheTag, +} from 'next/cache' +import { fetchData } from '../api/data' +// import { Suspense } from 'react' +// import { cookies, headers } from 'next/headers' + +function InnerComponent({ children }) { + return {children} +} + +async function refresh() { + 'use server' + revalidateTag('hello') +} + +async function reload() { + 'use server' +} + +async function Component() { + 'use cache' + cacheLife({ revalidate: 6 }) + unstable_cacheTag('hello') + return {await fetchData()} +} + +export default async function Home() { + return ( + <> +
+ +
+
+ +
+ + + ) +} diff --git a/test/development/app-dir/dynamic-io-dev-cache-scope/app/layout.tsx b/test/development/app-dir/dynamic-io-dev-cache-scope/app/layout.tsx new file mode 100644 index 00000000000000..745e32b8a8d235 --- /dev/null +++ b/test/development/app-dir/dynamic-io-dev-cache-scope/app/layout.tsx @@ -0,0 +1,9 @@ +export default function Root({ children }: { children: React.ReactNode }) { + return ( + + +
{children}
+ + + ) +} diff --git a/test/development/app-dir/dynamic-io-dev-cache-scope/app/uncached/page.tsx b/test/development/app-dir/dynamic-io-dev-cache-scope/app/uncached/page.tsx new file mode 100644 index 00000000000000..8a15e868310202 --- /dev/null +++ b/test/development/app-dir/dynamic-io-dev-cache-scope/app/uncached/page.tsx @@ -0,0 +1,33 @@ +import { fetchData } from '../api/data' +// import { Suspense } from 'react' +// import { cookies, headers } from 'next/headers' + +function InnerComponent({ children }) { + return {children} +} + +async function refresh() { + 'use server' +} + +async function reload() { + 'use server' +} + +async function Component() { + return {await fetchData()} +} + +export default async function Home() { + return ( + <> +
+ +
+
+ +
+ + + ) +} diff --git a/test/development/app-dir/dynamic-io-dev-cache-scope/dynamic-io-dev-cache-scope.test.ts b/test/development/app-dir/dynamic-io-dev-cache-scope/dynamic-io-dev-cache-scope.test.ts new file mode 100644 index 00000000000000..d1e08a035c1fd3 --- /dev/null +++ b/test/development/app-dir/dynamic-io-dev-cache-scope/dynamic-io-dev-cache-scope.test.ts @@ -0,0 +1,98 @@ +import { nextTestSetup } from 'e2e-utils' +import { + getRedboxDescription, + waitForAndOpenRuntimeError, + assertNoRedbox, + retry, + waitFor, +} from 'next-test-utils' + +describe('Dynamic IO Dev Errors', () => { + const { next } = nextTestSetup({ + files: __dirname, + }) + + it('should not show a red box error on the SSR render', async () => { + const browser = await next.browser('/cached') + await assertNoRedbox(browser) + let latestValue = await browser.elementByCss('#value').text() + + await browser.refresh() + await assertNoRedbox(browser) + let priorValue = latestValue + latestValue = await browser.elementByCss('#value').text() + + expect(latestValue).toBe(priorValue) + + await browser.elementByCss('#refresh').click() + await retry(async () => { + await assertNoRedbox(browser) + let priorValue = latestValue + latestValue = await browser.elementByCss('#value').text() + expect(latestValue).not.toBe(priorValue) + }) + + await browser.elementByCss('#refresh').click() + await retry(async () => { + await assertNoRedbox(browser) + let priorValue = latestValue + latestValue = await browser.elementByCss('#value').text() + expect(latestValue).not.toBe(priorValue) + }) + + // TODO CI is too flakey to run tests like this b/c timing cannot be controlled. + // we need to land this so we're deactivating these assertions for now. + // you should be able to reproduce the assertions below when testing locally + + // await browser.elementByCss('#reload').click() + // await retry(async () => { + // await assertNoRedbox(browser) + // let priorValue = latestValue + // latestValue = await browser.elementByCss('#value').text() + // expect(latestValue).toBe(priorValue) + // }) + + // await browser.elementByCss('#reload').click() + // await retry(async () => { + // await assertNoRedbox(browser) + // let priorValue = latestValue + // latestValue = await browser.elementByCss('#value').text() + // expect(latestValue).toBe(priorValue) + // }) + + // await browser.elementByCss('#refresh').click() + // await retry(async () => { + // await assertNoRedbox(browser) + // let priorValue = latestValue + // latestValue = await browser.elementByCss('#value').text() + // expect(latestValue).not.toBe(priorValue) + // }) + + await waitFor(6_000) + await browser.refresh() + await assertNoRedbox(browser) + priorValue = latestValue + latestValue = await browser.elementByCss('#value').text() + expect(latestValue).not.toBe(priorValue) + }) + + it('should show a red box error on the SSR render when data is uncached', async () => { + let desc + + const browser = await next.browser('/uncached') + await waitForAndOpenRuntimeError(browser) + desc = await getRedboxDescription(browser) + + expect(desc).toContain( + 'Route "/uncached": A component accessed data, headers, params, searchParams, or a short-lived cache without a Suspense boundary nor a "use cache" above it' + ) + + await browser.refresh() + await waitForAndOpenRuntimeError(browser) + desc = await getRedboxDescription(browser) + + expect(desc).toContain( + 'Route "/uncached": A component accessed data, headers, params, searchParams, or a short-lived cache without a Suspense boundary nor a "use cache" above it' + ) + }) +}) diff --git a/test/development/app-dir/dynamic-io-dev-cache-scope/next.config.js b/test/development/app-dir/dynamic-io-dev-cache-scope/next.config.js new file mode 100644 index 00000000000000..ac4afcf4321968 --- /dev/null +++ b/test/development/app-dir/dynamic-io-dev-cache-scope/next.config.js @@ -0,0 +1,10 @@ +/** + * @type {import('next').NextConfig} + */ +const nextConfig = { + experimental: { + dynamicIO: true, + }, +} + +module.exports = nextConfig diff --git a/test/e2e/app-dir/dynamic-io/dynamic-io.draft-mode.test.ts b/test/e2e/app-dir/dynamic-io/dynamic-io.draft-mode.test.ts index 612fcfaefb6dda..f1d5feac29243a 100644 --- a/test/e2e/app-dir/dynamic-io/dynamic-io.draft-mode.test.ts +++ b/test/e2e/app-dir/dynamic-io/dynamic-io.draft-mode.test.ts @@ -48,6 +48,8 @@ describe('dynamic-io', () => { expect.stringContaining('`draftMode().isEnabled`'), // TODO need to figure out why deduping isn't working here expect.stringContaining('`draftMode().isEnabled`'), + // TODO need to figure out why deduping isn't working here + expect.stringContaining('`draftMode().isEnabled`'), ]) } else { expect($('#layout').text()).toBe('at buildtime')