Skip to content

Commit

Permalink
[dynamicIO] Abort hanging promises from Request APIs after prerender …
Browse files Browse the repository at this point in the history
…is complete

When prerendering Request data access is gated through promises that never resolve. This is fine semantically but these promises can leak into other contexts such as `after`. Instead of leaving them hanging we will reject them once the prerender is complete.
  • Loading branch information
gnoff committed Oct 10, 2024
1 parent 5239813 commit 8026d66
Show file tree
Hide file tree
Showing 14 changed files with 285 additions and 12 deletions.
2 changes: 1 addition & 1 deletion packages/next/src/server/app-render/dynamic-rendering.ts
Original file line number Diff line number Diff line change
Expand Up @@ -570,7 +570,7 @@ export function useDynamicRouteParams(expression: string) {
// We are in a prerender with dynamicIO semantics
// We are going to hang here and never resolve. This will cause the currently
// rendering component to effectively be a dynamic hole
React.use(makeHangingPromise())
React.use(makeHangingPromise(workUnitStore.renderSignal, expression))
} else if (workUnitStore.type === 'prerender-ppr') {
// We're prerendering with PPR
postponeWithTracking(
Expand Down
22 changes: 18 additions & 4 deletions packages/next/src/server/dynamic-rendering-utils.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,26 @@
function hangForever() {}

/**
* This function constructs a promise that will never resolve. This is primarily
* useful for dynamicIO where we use promise resolution timing to determine which
* parts of a render can be included in a prerender.
*
* @internal
*/
export function makeHangingPromise<T>(): Promise<T> {
return new Promise(hangForever)
export function makeHangingPromise<T>(
signal: AbortSignal,
expression: string
): Promise<T> {
const hangingPromise = new Promise<T>((_, reject) => {
signal.addEventListener('abort', () => {
reject(
new Error(
`During prerendering, ${expression} rejects when the prerender is complete. Typically these errors are handled by React but if you move ${expression} to a different context by using \`setTimeout\`, \`after\`, or similar functions you may observe this error and you should handle it in that context.`
)
)
})
})
// We are fine if no one actually awaits this promise. We shouldn't consider this an unhandled rejection so
// we attach a noop catch handler here to suppress this warning. If you actually await somewhere or construct
// your own promise out of it you'll need to ensure you handle the error when it rejects.
hangingPromise.catch(() => {})
return hangingPromise
}
2 changes: 1 addition & 1 deletion packages/next/src/server/request/connection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ export function connection(): Promise<void> {
if (workUnitStore.type === 'prerender') {
// dynamicIO Prerender
// We return a promise that never resolves to allow the prender to stall at this point
return makeHangingPromise()
return makeHangingPromise(workUnitStore.renderSignal, '`connection()`')
} else if (workUnitStore.type === 'prerender-ppr') {
// PPR Prerender (no dynamicIO)
// We use React's postpone API to interrupt rendering here to create a dynamic hole
Expand Down
5 changes: 4 additions & 1 deletion packages/next/src/server/request/cookies.ts
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,10 @@ function makeDynamicallyTrackedExoticCookies(
return cachedPromise
}

const promise = makeHangingPromise<ReadonlyRequestCookies>()
const promise = makeHangingPromise<ReadonlyRequestCookies>(
prerenderStore.renderSignal,
'`cookies()`'
)
CachedCookies.set(prerenderStore, promise)

Object.defineProperties(promise, {
Expand Down
5 changes: 4 additions & 1 deletion packages/next/src/server/request/headers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,10 @@ function makeDynamicallyTrackedExoticHeaders(
return cachedHeaders
}

const promise = makeHangingPromise<ReadonlyHeaders>()
const promise = makeHangingPromise<ReadonlyHeaders>(
prerenderStore.renderSignal,
'`headers()`'
)
CachedHeaders.set(prerenderStore, promise)

Object.defineProperties(promise, {
Expand Down
7 changes: 5 additions & 2 deletions packages/next/src/server/request/params.ts
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ export function createPrerenderParamsForClientSegment(
// This params object has one of more fallback params so we need to consider
// the awaiting of this params object "dynamic". Since we are in dynamicIO mode
// we encode this as a promise that never resolves
return makeHangingPromise()
return makeHangingPromise(prerenderStore.renderSignal, '`params`')
}
}
}
Expand Down Expand Up @@ -190,7 +190,10 @@ function makeAbortingExoticParams(
return cachedParams
}

const promise = makeHangingPromise<Params>()
const promise = makeHangingPromise<Params>(
prerenderStore.renderSignal,
'`params`'
)
CachedParams.set(underlyingParams, promise)

Object.keys(underlyingParams).forEach((prop) => {
Expand Down
7 changes: 5 additions & 2 deletions packages/next/src/server/request/search-params.ts
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ export function createPrerenderSearchParamsForClientPage(
// dynamicIO Prerender
// We're prerendering in a mode that aborts (dynamicIO) and should stall
// the promise to ensure the RSC side is considered dynamic
return makeHangingPromise()
return makeHangingPromise(prerenderStore.renderSignal, '`searchParams`')
}
// We're prerendering in a mode that does not aborts. We resolve the promise without
// any tracking because we're just transporting a value from server to client where the tracking
Expand Down Expand Up @@ -165,7 +165,10 @@ function makeAbortingExoticSearchParams(
return cachedSearchParams
}

const promise = makeHangingPromise<SearchParams>()
const promise = makeHangingPromise<SearchParams>(
prerenderStore.renderSignal,
'`searchParams`'
)

const proxiedPromise = new Proxy(promise, {
get(target, prop, receiver) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,126 @@
import { nextTestSetup } from 'e2e-utils'

const WITH_PPR = !!process.env.__NEXT_EXPERIMENTAL_PPR

const stackStart = /\s+at /

function createExpectError(cliOutput: string) {
let cliIndex = 0
return function expectError(
containing: string,
withStackContaining?: string
) {
const initialCliIndex = cliIndex
let lines = cliOutput.slice(cliIndex).split('\n')

let i = 0
while (i < lines.length) {
let line = lines[i++] + '\n'
cliIndex += line.length
if (line.includes(containing)) {
if (typeof withStackContaining !== 'string') {
return
} else {
while (i < lines.length) {
let stackLine = lines[i++] + '\n'
if (!stackStart.test(stackLine)) {
expect(stackLine).toContain(withStackContaining)
}
if (stackLine.includes(withStackContaining)) {
return
}
}
}
}
}

expect(cliOutput.slice(initialCliIndex)).toContain(containing)
}
}

describe(`Request Promises`, () => {
describe('On Prerender Completion', () => {
const { next, isNextDev, skipped } = nextTestSetup({
files: __dirname + '/fixtures/reject-hanging-promises-static',
skipStart: true,
})

if (skipped) {
return
}

if (isNextDev) {
it('does not run in dev', () => {})
return
}

it('should reject request APIs after the prerender is complete when it finishes naturally', async () => {
try {
await next.start()
} catch {
throw new Error('expected build not to fail for fully static project')
}
const expectError = createExpectError(next.cliOutput)

if (WITH_PPR) {
expectError(
'Error: During prerendering, `params` rejects when the prerender is complete'
)
}
expectError(
'Error: During prerendering, `searchParams` rejects when the prerender is complete'
)
expectError(
'Error: During prerendering, `cookies()` rejects when the prerender is complete'
)
expectError(
'Error: During prerendering, `headers()` rejects when the prerender is complete'
)
expectError(
'Error: During prerendering, `connection()` rejects when the prerender is complete'
)
})
})
describe('On Prerender Interruption', () => {
const { next, isNextDev, skipped } = nextTestSetup({
files: __dirname + '/fixtures/reject-hanging-promises-dynamic',
skipStart: true,
})

if (skipped) {
return
}

if (isNextDev) {
it('does not run in dev', () => {})
return
}

it('should reject request APIs after the prerender is interrupted with synchronously dynamic APIs', async () => {
try {
await next.start()
} catch {
throw new Error('expected build not to fail for fully static project')
}
const expectError = createExpectError(next.cliOutput)

if (WITH_PPR) {
expectError(
'Error: During prerendering, `params` rejects when the prerender is complete'
)
}
expectError(
'Error: During prerendering, `searchParams` rejects when the prerender is complete'
)
expectError(
'Error: During prerendering, `cookies()` rejects when the prerender is complete'
)
expectError(
'Error: During prerendering, `headers()` rejects when the prerender is complete'
)
expectError(
'Error: During prerendering, `connection()` rejects when the prerender is complete'
)
})
})
})
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
import { cookies, headers, draftMode } from 'next/headers'
import { connection } from 'next/server'

export function generateStaticParams() {
return [{ slug: 'one' }]
}

export default async function Page(props: {
params: Promise<{}>
searchParams: Promise<{}>
}) {
setTimeout(async () => await props.params)
setTimeout(async () => await props.searchParams)
let pendingCookies = cookies()
setTimeout(async () => await pendingCookies)
let pendingHeaders = headers()
setTimeout(async () => await pendingHeaders)
let pendingDraftMode = draftMode()
setTimeout(async () => await pendingDraftMode)
let pendingConnection = connection()
setTimeout(async () => await pendingConnection)
return (
<>
<p>
This page renders statically but it passes all of the Request Data
promises (cookies(), etc...) to a setTimeout scope. This test asserts
that these promises eventually reject even when the route is
synchronously dynamic (which this one is by rendering a Math.random()
value)
</p>
<p>
<TriggerSyncDynamic />
</p>
</>
)
}

async function TriggerSyncDynamic() {
await new Promise((r) => process.nextTick(r))
return Math.random()
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import { Suspense } from 'react'

export default function Root({ children }: { children: React.ReactNode }) {
return (
<html>
<body>
<main>
<Suspense fallback="loading...">{children}</Suspense>
</main>
</body>
</html>
)
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
/**
* @type {import('next').NextConfig}
*/
const nextConfig = {
experimental: {
ppr: process.env.__NEXT_EXPERIMENTAL_PPR === 'true',
pprFallbacks: process.env.__NEXT_EXPERIMENTAL_PPR === 'true',
dynamicIO: true,
serverMinification: true,
},
}

module.exports = nextConfig
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
import { cookies, headers, draftMode } from 'next/headers'
import { connection } from 'next/server'

export function generateStaticParams() {
return [{ slug: 'one' }]
}

export default async function Page(props: {
params: Promise<{}>
searchParams: Promise<{}>
}) {
setTimeout(async () => await props.params)
setTimeout(async () => await props.searchParams)
let pendingCookies = cookies()
setTimeout(async () => await pendingCookies)
let pendingHeaders = headers()
setTimeout(async () => await pendingHeaders)
let pendingDraftMode = draftMode()
setTimeout(async () => await pendingDraftMode)
let pendingConnection = connection()
setTimeout(async () => await pendingConnection)
return (
<>
<p>
This page renders statically but it passes all of the Request Data
promises (cookies(), etc...) to a setTimeout scope. This test asserts
that these promises eventually reject even when the route is entirely
static (which this one is)
</p>
</>
)
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
export default function Root({ children }: { children: React.ReactNode }) {
return (
<html>
<body>
<main>{children}</main>
</body>
</html>
)
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
/**
* @type {import('next').NextConfig}
*/
const nextConfig = {
experimental: {
ppr: process.env.__NEXT_EXPERIMENTAL_PPR === 'true',
pprFallbacks: process.env.__NEXT_EXPERIMENTAL_PPR === 'true',
dynamicIO: true,
serverMinification: true,
},
}

module.exports = nextConfig

0 comments on commit 8026d66

Please sign in to comment.