Skip to content

Commit 2ad2fc0

Browse files
lubieowocelubieowoce
committed
fixing hanging cache deadlock
1 parent 50cbb16 commit 2ad2fc0

File tree

10 files changed

+210
-35
lines changed

10 files changed

+210
-35
lines changed

‎packages/next/src/server/app-render/app-render.tsx‎

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2767,7 +2767,7 @@ async function renderWithRestartOnCacheMissInDev(
27672767
initialRequestStore: RequestStore,
27682768
createRequestStore: () => RequestStore,
27692769
getPayload: (requestStore: RequestStore) => Promise<RSCPayload>,
2770-
onError: (error: unknown) => void
2770+
onError: (err: unknown) => string | undefined
27712771
) {
27722772
const {
27732773
htmlRequestId,
@@ -2827,9 +2827,9 @@ async function renderWithRestartOnCacheMissInDev(
28272827
const prerenderResumeDataCache = createPrerenderResumeDataCache()
28282828

28292829
const initialReactController = new AbortController()
2830-
const initialDataController = new AbortController() // Controls hanging promises we create
2830+
const initialHangingPromiseController = new AbortController()
28312831
const initialStageController = new StagedRenderingController(
2832-
initialDataController.signal
2832+
initialHangingPromiseController.signal
28332833
)
28342834

28352835
requestStore.prerenderResumeDataCache = prerenderResumeDataCache
@@ -2858,7 +2858,13 @@ async function renderWithRestartOnCacheMissInDev(
28582858
initialRscPayload,
28592859
clientReferenceManifest.clientModules,
28602860
{
2861-
onError,
2861+
onError: (err) => {
2862+
if (initialReactController.signal.aborted) {
2863+
return undefined
2864+
} else {
2865+
return onError(err)
2866+
}
2867+
},
28622868
environmentName,
28632869
filterStackFrame,
28642870
debugChannel: debugChannel?.serverSide,
@@ -2869,7 +2875,8 @@ async function renderWithRestartOnCacheMissInDev(
28692875
// Note that we want to install this listener after the render is started
28702876
// so that it runs after react is finished running its abort code.
28712877
initialReactController.signal.addEventListener('abort', () => {
2872-
initialDataController.abort(initialReactController.signal.reason)
2878+
const { reason } = initialReactController.signal
2879+
initialHangingPromiseController.abort(reason)
28732880
})
28742881
return stream
28752882
},

‎packages/next/src/server/dynamic-rendering-utils.ts‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ export function makeHangingPromise<T>(
4242
expression: string
4343
): Promise<T> {
4444
if (signal.aborted) {
45-
return Promise.reject(new HangingPromiseRejectionError(route, expression))
45+
return makeRejectedHangingPromise(route, expression)
4646
} else {
4747
const hangingPromise = new Promise<T>((_, reject) => {
4848
const boundRejection = reject.bind(
@@ -74,6 +74,13 @@ export function makeHangingPromise<T>(
7474
}
7575
}
7676

77+
export function makeRejectedHangingPromise(
78+
route: string,
79+
expression: string
80+
): Promise<never> {
81+
return Promise.reject(new HangingPromiseRejectionError(route, expression))
82+
}
83+
7784
function ignoreReject() {}
7885

7986
export function makeDevtoolsIOAwarePromise<T>(

‎packages/next/src/server/use-cache/use-cache-wrapper.ts‎

Lines changed: 107 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -38,8 +38,8 @@ import {
3838
} from '../app-render/work-unit-async-storage.external'
3939

4040
import {
41-
makeDevtoolsIOAwarePromise,
4241
makeHangingPromise,
42+
makeRejectedHangingPromise,
4343
} from '../dynamic-rendering-utils'
4444

4545
import type { ClientReferenceManifestForRsc } from '../../build/webpack/plugins/flight-manifest-plugin'
@@ -71,7 +71,10 @@ import { createLazyResult, isResolvedLazyResult } from '../lib/lazy-result'
7171
import { dynamicAccessAsyncStorage } from '../app-render/dynamic-access-async-storage.external'
7272
import { isReactLargeShellError } from '../app-render/react-large-shell-error'
7373
import type { CacheLife } from './cache-life'
74-
import { RenderStage } from '../app-render/staged-rendering'
74+
import {
75+
RenderStage,
76+
type NonStaticRenderStage,
77+
} from '../app-render/staged-rendering'
7578

7679
interface PrivateCacheContext {
7780
readonly kind: 'private'
@@ -1015,10 +1018,9 @@ export function cache(
10151018
if (process.env.NODE_ENV === 'development') {
10161019
// Similar to runtime prerenders, private caches should not resolve in the static stage
10171020
// of a dev request, so we delay them.
1018-
await makeDevtoolsIOAwarePromise(
1019-
undefined,
1020-
outerWorkUnitStore,
1021-
RenderStage.Runtime
1021+
await delayBeforeCacheReadStartInDev(
1022+
RenderStage.Runtime,
1023+
outerWorkUnitStore
10221024
)
10231025
}
10241026
break
@@ -1280,14 +1282,19 @@ export function cache(
12801282
// We delay the cache here so that it doesn't resolve in the static task --
12811283
// in a regular static prerender, it'd be a hanging promise, and we need to reflect that,
12821284
// so it has to resolve later.
1283-
// TODO(restart-on-cache-miss): Optimize this to avoid unnecessary restarts.
1284-
// We don't end the cache read here, so this will always appear as a cache miss in the static stage,
1285-
// and thus will cause a restart even if all caches are filled.
1286-
await makeDevtoolsIOAwarePromise(
1287-
undefined,
1285+
// TODO(restart-on-cache-miss): This can fallthrough to the `RUNTIME_PREFETCH_DYNAMIC_STALE`
1286+
// check below, and try to delay again. This is not incorrect, but it's unnecessary.
1287+
// refactor this to avoid it.
1288+
const hang = await delayOrHangStartedCacheReadInDev(
1289+
RenderStage.Runtime,
12881290
workUnitStore,
1289-
RenderStage.Runtime
1291+
cacheSignal,
1292+
workStore.route,
1293+
'dynamic "use cache"'
12901294
)
1295+
if (hang) {
1296+
return hang.hangingPromise
1297+
}
12911298
}
12921299
break
12931300
}
@@ -1322,14 +1329,16 @@ export function cache(
13221329
// We delay the cache here so that it doesn't resolve in the runtime phase --
13231330
// in a regular runtime prerender, it'd be a hanging promise, and we need to reflect that,
13241331
// so it has to resolve later.
1325-
// TODO(restart-on-cache-miss): Optimize this to avoid unnecessary restarts.
1326-
// We don't end the cache read here, so this will always appear as a cache miss in the runtime stage,
1327-
// and thus will cause a restart even if all caches are filled.
1328-
await makeDevtoolsIOAwarePromise(
1329-
undefined,
1332+
const hang = await delayOrHangStartedCacheReadInDev(
1333+
RenderStage.Dynamic,
13301334
workUnitStore,
1331-
RenderStage.Dynamic
1335+
cacheSignal,
1336+
workStore.route,
1337+
'dynamic "use cache"'
13321338
)
1339+
if (hang) {
1340+
return hang.hangingPromise
1341+
}
13331342
}
13341343
break
13351344
}
@@ -1496,14 +1505,16 @@ export function cache(
14961505
// We delay the cache here so that it doesn't resolve in the static task --
14971506
// in a regular static prerender, it'd be a hanging promise, and we need to reflect that,
14981507
// so it has to resolve later.
1499-
// TODO(restart-on-cache-miss): Optimize this to avoid unnecessary restarts.
1500-
// We don't end the cache read here, so this will always appear as a cache miss in the static stage,
1501-
// and thus will cause a restart even if all caches are filled.
1502-
await makeDevtoolsIOAwarePromise(
1503-
undefined,
1508+
const hang = await delayOrHangStartedCacheReadInDev(
1509+
RenderStage.Dynamic,
15041510
workUnitStore,
1505-
RenderStage.Runtime
1511+
cacheSignal,
1512+
workStore.route,
1513+
'dynamic "use cache"'
15061514
)
1515+
if (hang) {
1516+
return hang.hangingPromise
1517+
}
15071518
}
15081519
break
15091520
}
@@ -1848,3 +1859,75 @@ function isRecentlyRevalidatedTag(tag: string, workStore: WorkStore): boolean {
18481859

18491860
return false
18501861
}
1862+
1863+
async function delayBeforeCacheReadStartInDev(
1864+
stage: NonStaticRenderStage,
1865+
requestStore: RequestStore
1866+
): Promise<void> {
1867+
const { stagedRendering } = requestStore
1868+
if (stagedRendering && stagedRendering.currentStage < stage) {
1869+
await stagedRendering.waitForStage(stage)
1870+
}
1871+
}
1872+
1873+
/** Note: Only call this after a `cacheSignal.beginRead()`. */
1874+
async function delayOrHangStartedCacheReadInDev(
1875+
stage: NonStaticRenderStage,
1876+
requestStore: RequestStore,
1877+
cacheSignal: CacheSignal | null,
1878+
route: string,
1879+
expression: string
1880+
): Promise<{ hangingPromise: Promise<never> } | null> {
1881+
const { stagedRendering } = requestStore
1882+
// No staging, so we don't need to delay.
1883+
if (!stagedRendering) {
1884+
return null
1885+
}
1886+
1887+
// We're already at or beyond the target stage, so we shouldn't hang or delay.
1888+
if (stagedRendering.currentStage >= stage) {
1889+
return null
1890+
}
1891+
1892+
// We've got an ongoing cache read that can only resolve in a future stage.
1893+
1894+
if (cacheSignal) {
1895+
// We're filling caches, and might need to omit this one if we can't reach its target stage.
1896+
// This can happen e.g. if a cache only resolves in the Dynamic stage --
1897+
// we never advance to it in a cache filling render.
1898+
1899+
// Hide the cache read.
1900+
// It won't resolve in the current stage anyway,
1901+
// so we don't want it to show up when we check if there's pending reads
1902+
// (i.e. cache misses) at the beginning of the next stage.
1903+
// Otherwise, the render might be bailed out and used as a warmup,
1904+
// and we'd stop advancing stages (e.g. we would never reach Dynamic)
1905+
// so this read would never end, and `cacheSignal.cacheReady()` would deadlock.
1906+
cacheSignal.endRead()
1907+
1908+
// We haven't reached the target stage yet, so we wait for one of these two things:
1909+
try {
1910+
await stagedRendering.waitForStage(stage)
1911+
// 1. We reach the target stage, and we can unblock the cacke
1912+
1913+
// We need to restart the read that we hid before, since it hasn't actually finished.
1914+
cacheSignal.beginRead()
1915+
1916+
return null
1917+
} catch {
1918+
// 2. The render is aborted before we reached the target stage.
1919+
1920+
// The render was already aborted, so we can just return a rejected promise instead of a hanging one.
1921+
const hangingPromise = makeRejectedHangingPromise(route, expression)
1922+
// Wrapped in an object so that we can return it without it being awaited by the runtime.
1923+
return { hangingPromise }
1924+
}
1925+
}
1926+
1927+
cacheSignal satisfies null
1928+
1929+
// Otherwise, we're in the restarted render, after caches have been filled.
1930+
// This render should finish completely, without aborting, so we should only delay.
1931+
await stagedRendering.waitForStage(stage)
1932+
return null
1933+
}

‎test/development/app-dir/cache-components-dev-warmup/cache-components.dev-warmup.test.ts‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -246,14 +246,20 @@ describe.each([
246246
assertLog(logs, 'after cache read - page', 'Prerender')
247247

248248
// Short lived caches are dynamic holes in static prerenders,
249-
// so they shouldn't resolve in the static stage.
249+
// so they shouldn't resolve in the static stage,
250+
// but they should resolve in the runtime stage.
250251
assertLog(logs, 'after short-lived cache read - page', RUNTIME_ENV)
251252
assertLog(
252253
logs,
253254
'after short-lived cache read - layout',
254255
RUNTIME_ENV
255256
)
256257

258+
// Dynamic caches are holes in static and runtime prerenders,
259+
// so they should only resolve in the dynamic stage.
260+
assertLog(logs, 'after dynamic cache read - layout', 'Server')
261+
assertLog(logs, 'after dynamic cache read - page', 'Server')
262+
257263
assertLog(logs, 'after uncached fetch - layout', 'Server')
258264
assertLog(logs, 'after uncached fetch - page', 'Server')
259265
}

‎test/development/app-dir/cache-components-dev-warmup/fixtures/with-prefetch-config/app/short-lived-cache/data-fetching.tsx‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,3 +23,31 @@ async function getShortLivedCachedData(_key: string) {
2323
await new Promise((r) => setTimeout(r))
2424
return Math.random()
2525
}
26+
27+
export async function DynamicCache({
28+
label,
29+
cacheKey,
30+
}: {
31+
label: string
32+
cacheKey: string
33+
}) {
34+
const data = await getDynamicCachedData(cacheKey)
35+
console.log(`after dynamic cache read - ${label}`)
36+
return (
37+
<dl>
38+
<dt>Dynamic Cached Data (Page)</dt>
39+
<dd>{data}</dd>
40+
</dl>
41+
)
42+
}
43+
44+
async function getDynamicCachedData(_key: string) {
45+
'use cache'
46+
cacheLife({
47+
stale: 20, // < DYNAMIC_STALE (excluded from runtime prerenders)
48+
revalidate: 2 * 60,
49+
expire: 3 * 60, // < DYNAMIC_EXPIRE (excluded from static prerenders)
50+
})
51+
await new Promise((r) => setTimeout(r))
52+
return Math.random()
53+
}

‎test/development/app-dir/cache-components-dev-warmup/fixtures/with-prefetch-config/app/short-lived-cache/layout.tsx‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { Suspense } from 'react'
22
import { UncachedFetch, CachedData } from '../data-fetching'
3-
import { ShortLivedCache } from './data-fetching'
3+
import { DynamicCache, ShortLivedCache } from './data-fetching'
44

55
export const unstable_prefetch = { mode: 'runtime', samples: [{}] }
66

@@ -20,6 +20,10 @@ export default function Layout({ children }: { children: React.ReactNode }) {
2020
<ShortLivedCache label="layout" cacheKey={CACHE_KEY} />
2121
</Suspense>
2222

23+
<Suspense fallback="Loading dynamic cache...">
24+
<DynamicCache label="layout" cacheKey={CACHE_KEY} />
25+
</Suspense>
26+
2327
<Suspense fallback="Loading uncached fetch...">
2428
<UncachedFetch label="layout" cacheKey={CACHE_KEY} />
2529
</Suspense>

‎test/development/app-dir/cache-components-dev-warmup/fixtures/with-prefetch-config/app/short-lived-cache/page.tsx‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { Suspense } from 'react'
22
import { CachedData, UncachedFetch } from '../data-fetching'
3-
import { ShortLivedCache } from './data-fetching'
3+
import { DynamicCache, ShortLivedCache } from './data-fetching'
44

55
const CACHE_KEY = __dirname + '/__PAGE__'
66

@@ -15,6 +15,10 @@ export default async function Page() {
1515
<ShortLivedCache label="page" cacheKey={CACHE_KEY} />
1616
</Suspense>
1717

18+
<Suspense fallback="Loading dynamic cache...">
19+
<DynamicCache label="page" cacheKey={CACHE_KEY} />
20+
</Suspense>
21+
1822
<Suspense fallback="Loading uncached fetch...">
1923
<UncachedFetch label="page" cacheKey={CACHE_KEY} />
2024
</Suspense>

‎test/development/app-dir/cache-components-dev-warmup/fixtures/without-prefetch-config/app/short-lived-cache/data-fetching.tsx‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,3 +23,31 @@ async function getShortLivedCachedData(_key: string) {
2323
await new Promise((r) => setTimeout(r))
2424
return Math.random()
2525
}
26+
27+
export async function DynamicCache({
28+
label,
29+
cacheKey,
30+
}: {
31+
label: string
32+
cacheKey: string
33+
}) {
34+
const data = await getDynamicCachedData(cacheKey)
35+
console.log(`after dynamic cache read - ${label}`)
36+
return (
37+
<dl>
38+
<dt>Dynamic Cached Data (Page)</dt>
39+
<dd>{data}</dd>
40+
</dl>
41+
)
42+
}
43+
44+
async function getDynamicCachedData(_key: string) {
45+
'use cache'
46+
cacheLife({
47+
stale: 20, // < DYNAMIC_STALE (excluded from runtime prerenders)
48+
revalidate: 2 * 60,
49+
expire: 3 * 60, // < DYNAMIC_EXPIRE (excluded from static prerenders)
50+
})
51+
await new Promise((r) => setTimeout(r))
52+
return Math.random()
53+
}

‎test/development/app-dir/cache-components-dev-warmup/fixtures/without-prefetch-config/app/short-lived-cache/layout.tsx‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { Suspense } from 'react'
22
import { UncachedFetch, CachedData } from '../data-fetching'
3-
import { ShortLivedCache } from './data-fetching'
3+
import { DynamicCache, ShortLivedCache } from './data-fetching'
44

55
const CACHE_KEY = __dirname + '/__LAYOUT__'
66

@@ -18,6 +18,10 @@ export default function Layout({ children }: { children: React.ReactNode }) {
1818
<ShortLivedCache label="layout" cacheKey={CACHE_KEY} />
1919
</Suspense>
2020

21+
<Suspense fallback="Loading dynamic cache...">
22+
<DynamicCache label="layout" cacheKey={CACHE_KEY} />
23+
</Suspense>
24+
2125
<Suspense fallback="Loading uncached fetch...">
2226
<UncachedFetch label="layout" cacheKey={CACHE_KEY} />
2327
</Suspense>

0 commit comments

Comments
 (0)