Skip to content

Commit

Permalink
Associate server error digest with browser logged one (#61592)
Browse files Browse the repository at this point in the history
### What

#### Core
This PR respect the error's digest when recieves new error occurred from
server side, and it will be logged into client on production with the
same `digest` property.
If we discover the original RSC error in SSR error handler, retrieve the
original error

#### Tests

* Move the errors related tests from `test/e2e/app-dir/app` to a
separate test suite `test/e2e/app-dir/errors`
* Add a new test case for logging the original RSC error
* Add a new test case for logging the original Server Action error


### Why

This will help associate the `digest` property of the errors logged from
client with the actual generated server errors. Previously they're
different as we might re-compute the digest proper in handler that react
server renderer thinks it's a new error, which causes we have 2
different errors logged on server side, and 1 logged on client side. The
one on client side can associate to the server errors but it's from
react renderer which is not the original error.

Closes NEXT-2094
Fixes #60684
  • Loading branch information
huozhi committed Feb 6, 2024
1 parent 89fcf68 commit 92e4a4b
Show file tree
Hide file tree
Showing 18 changed files with 279 additions and 159 deletions.
32 changes: 19 additions & 13 deletions packages/next/src/server/app-render/app-render.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,11 @@ import { addImplicitTags } from '../lib/patch-fetch'
import { AppRenderSpan } from '../lib/trace/constants'
import { getTracer } from '../lib/trace/tracer'
import { FlightRenderResult } from './flight-render-result'
import { createErrorHandler, type ErrorHandler } from './create-error-handler'
import {
createErrorHandler,
ErrorHandlerSource,
type ErrorHandler,
} from './create-error-handler'
import {
getShortDynamicParamType,
dynamicParamTypes,
Expand Down Expand Up @@ -627,7 +631,7 @@ async function renderToHTMLOrFlightImpl(
serverModuleMap,
})

const capturedErrors: Error[] = []
const digestErrorsMap: Map<string, Error> = new Map()
const allCapturedErrors: Error[] = []
const isNextExport = !!renderOpts.nextExport
const { staticGenerationStore, requestStore } = baseCtx
Expand All @@ -638,27 +642,27 @@ async function renderToHTMLOrFlightImpl(
renderOpts.experimental.ppr && isStaticGeneration

const serverComponentsErrorHandler = createErrorHandler({
_source: 'serverComponentsRenderer',
source: ErrorHandlerSource.serverComponents,
dev,
isNextExport,
errorLogger: appDirDevErrorLogger,
capturedErrors,
digestErrorsMap,
silenceLogger: silenceStaticGenerationErrors,
})
const flightDataRendererErrorHandler = createErrorHandler({
_source: 'flightDataRenderer',
source: ErrorHandlerSource.flightData,
dev,
isNextExport,
errorLogger: appDirDevErrorLogger,
capturedErrors,
digestErrorsMap,
silenceLogger: silenceStaticGenerationErrors,
})
const htmlRendererErrorHandler = createErrorHandler({
_source: 'htmlRenderer',
source: ErrorHandlerSource.html,
dev,
isNextExport,
errorLogger: appDirDevErrorLogger,
capturedErrors,
digestErrorsMap,
allCapturedErrors,
silenceLogger: silenceStaticGenerationErrors,
})
Expand Down Expand Up @@ -987,7 +991,7 @@ async function renderToHTMLOrFlightImpl(
}

return { stream }
} catch (err) {
} catch (err: any) {
if (
isStaticGenBailoutError(err) ||
(typeof err === 'object' &&
Expand Down Expand Up @@ -1226,6 +1230,8 @@ async function renderToHTMLOrFlightImpl(
// It got here, which means it did not reject, so clear the timeout to avoid
// it from rejecting again (which is a no-op anyways).
clearTimeout(timeout)
const buildFailingError =
digestErrorsMap.size > 0 ? digestErrorsMap.values().next().value : null

// If PPR is enabled and the postpone was triggered but lacks the postponed
// state information then we should error out unless the client side rendering
Expand All @@ -1247,12 +1253,12 @@ async function renderToHTMLOrFlightImpl(
`by your own try/catch. Learn more: https://nextjs.org/docs/messages/ppr-caught-error`
)

if (capturedErrors.length > 0) {
if (digestErrorsMap.size > 0) {
warn(
'The following error was thrown during build, and may help identify the source of the issue:'
)

error(capturedErrors[0])
error(buildFailingError)
}

throw new MissingPostponeDataError(
Expand All @@ -1268,8 +1274,8 @@ async function renderToHTMLOrFlightImpl(

// If we encountered any unexpected errors during build we fail the
// prerendering phase and the build.
if (capturedErrors.length > 0) {
throw capturedErrors[0]
if (buildFailingError) {
throw buildFailingError
}

// Wait for and collect the flight payload data if we don't have it
Expand Down
45 changes: 35 additions & 10 deletions packages/next/src/server/app-render/create-error-handler.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,16 @@ import { SpanStatusCode, getTracer } from '../lib/trace/tracer'
import { isAbortError } from '../pipe-readable'
import { isDynamicUsageError } from '../../export/helpers/is-dynamic-usage-error'

export type ErrorHandler = (err: any) => string | undefined
export type ErrorHandler = (
err: unknown,
errorInfo: unknown
) => string | undefined

export const ErrorHandlerSource = {
serverComponents: 'serverComponents',
flightData: 'flightData',
html: 'html',
} as const

/**
* Create error handler for renderers.
Expand All @@ -15,23 +24,33 @@ export function createErrorHandler({
/**
* Used for debugging
*/
_source,
source,
dev,
isNextExport,
errorLogger,
capturedErrors,
digestErrorsMap,
allCapturedErrors,
silenceLogger,
}: {
_source: string
source: (typeof ErrorHandlerSource)[keyof typeof ErrorHandlerSource]
dev?: boolean
isNextExport?: boolean
errorLogger?: (err: any) => Promise<void>
capturedErrors: Error[]
digestErrorsMap: Map<string, Error>
allCapturedErrors?: Error[]
silenceLogger?: boolean
}): ErrorHandler {
return (err) => {
return (err: any, errorInfo: any) => {
// If the error already has a digest, respect the original digest,
// so it won't get re-generated into another new error.
if (!err.digest) {
// TODO-APP: look at using webcrypto instead. Requires a promise to be awaited.
err.digest = stringHash(
err.message + (errorInfo?.stack || err.stack || '')
).toString()
}
const digest = err.digest

if (allCapturedErrors) allCapturedErrors.push(err)

// These errors are expected. We return the digest
Expand All @@ -41,12 +60,20 @@ export function createErrorHandler({
// If the response was closed, we don't need to log the error.
if (isAbortError(err)) return

if (!digestErrorsMap.has(digest)) {
digestErrorsMap.set(digest, err)
} else if (source === ErrorHandlerSource.html) {
// For SSR errors, if we have the existing digest in errors map,
// we should use the existing error object to avoid duplicate error logs.
err = digestErrorsMap.get(digest)
}

// Format server errors in development to add more helpful error messages
if (dev) {
formatServerError(err)
}
// Used for debugging error source
// console.error(_source, err)
// console.error(source, err)
// Don't log the suppressed error during export
if (
!(
Expand Down Expand Up @@ -84,8 +111,6 @@ export function createErrorHandler({
}
}

capturedErrors.push(err)
// TODO-APP: look at using webcrypto instead. Requires a promise to be awaited.
return stringHash(err.message + err.stack + (err.digest || '')).toString()
return err.digest
}
}
10 changes: 5 additions & 5 deletions packages/next/types/react-dom.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ declare module 'react-dom/server.edge' {
export type ResumeOptions = {
nonce?: string
signal?: AbortSignal
onError?: (error: unknown) => string | undefined
onError?: (error: unknown, errorInfo: unknown) => string | undefined
onPostpone?: (reason: string) => void
unstable_externalRuntimeSrc?: string | BootstrapScriptDescriptor
}
Expand All @@ -28,7 +28,7 @@ declare module 'react-dom/server.edge' {
children: JSX.Element,
postponedState: object,
options?: {
onError?: (error: Error) => void
onError?: (error: Error, errorInfo: unknown) => void
}
): Promise<ReadableStream<Uint8Array>>

Expand All @@ -46,7 +46,7 @@ declare module 'react-dom/server.edge' {
bootstrapModules?: Array<string | BootstrapScriptDescriptor>
progressiveChunkSize?: number
signal?: AbortSignal
onError?: (error: unknown) => string | undefined
onError?: (error: unknown, errorInfo: unknown) => string | undefined
onPostpone?: (reason: string) => void
unstable_externalRuntimeSrc?: string | BootstrapScriptDescriptor
importMap?: {
Expand Down Expand Up @@ -97,7 +97,7 @@ declare module 'react-dom/static.edge' {
bootstrapModules?: Array<string | BootstrapScriptDescriptor>
progressiveChunkSize?: number
signal?: AbortSignal
onError?: (error: unknown) => string | undefined
onError?: (error: unknown, errorInfo: unknown) => string | undefined
onPostpone?: (reason: string) => void
unstable_externalRuntimeSrc?: string | BootstrapScriptDescriptor
importMap?: {
Expand All @@ -117,7 +117,7 @@ declare module 'react-dom/static.edge' {
export function prerender(
children: JSX.Element,
options?: {
onError?: (error: Error) => void
onError?: (error: Error, errorInfo: unknown) => void
onHeaders?: (headers: Headers) => void
}
): Promise<{
Expand Down
132 changes: 1 addition & 131 deletions test/e2e/app-dir/app/index.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { createNextDescribe } from 'e2e-utils'
import { check, getRedboxHeader, hasRedbox, waitFor } from 'next-test-utils'
import { check, waitFor } from 'next-test-utils'
import cheerio from 'cheerio'
import stripAnsi from 'strip-ansi'

Expand Down Expand Up @@ -1394,136 +1394,6 @@ createNextDescribe(
)
})

describe('error component', () => {
it('should trigger error component when an error happens during rendering', async () => {
const browser = await next.browser('/error/client-component')
await browser.elementByCss('#error-trigger-button').click()

if (isDev) {
// TODO: investigate desired behavior here as it is currently
// minimized by default
// expect(await hasRedbox(browser)).toBe(true)
// expect(await getRedboxHeader(browser)).toMatch(/this is a test/)
} else {
await browser
expect(
await browser
.waitForElementByCss('#error-boundary-message')
.elementByCss('#error-boundary-message')
.text()
).toBe('An error occurred: this is a test')
}
})

it('should trigger error component when an error happens during server components rendering', async () => {
const browser = await next.browser('/error/server-component')

if (isDev) {
expect(
await browser
.waitForElementByCss('#error-boundary-message')
.elementByCss('#error-boundary-message')
.text()
).toBe('this is a test')
expect(
await browser.waitForElementByCss('#error-boundary-digest').text()
// Digest of the error message should be stable.
).not.toBe('')
// TODO-APP: ensure error overlay is shown for errors that happened before/during hydration
// expect(await hasRedbox(browser)).toBe(true)
// expect(await getRedboxHeader(browser)).toMatch(/this is a test/)
} else {
await browser
expect(
await browser.waitForElementByCss('#error-boundary-message').text()
).toBe(
'An error occurred in the Server Components render. The specific message is omitted in production builds to avoid leaking sensitive details. A digest property is included on this error instance which may provide additional details about the nature of the error.'
)
expect(
await browser.waitForElementByCss('#error-boundary-digest').text()
// Digest of the error message should be stable.
).not.toBe('')
}
})

it('should use default error boundary for prod and overlay for dev when no error component specified', async () => {
const browser = await next.browser(
'/error/global-error-boundary/client'
)
await browser.elementByCss('#error-trigger-button').click()

if (isDev) {
expect(await hasRedbox(browser)).toBe(true)
expect(await getRedboxHeader(browser)).toMatch(/this is a test/)
} else {
expect(
await browser.waitForElementByCss('body').elementByCss('h2').text()
).toBe(
'Application error: a client-side exception has occurred (see the browser console for more information).'
)
}
})

it('should display error digest for error in server component with default error boundary', async () => {
const browser = await next.browser(
'/error/global-error-boundary/server'
)

if (isDev) {
expect(await hasRedbox(browser)).toBe(true)
expect(await getRedboxHeader(browser)).toMatch(/custom server error/)
} else {
expect(
await browser.waitForElementByCss('body').elementByCss('h2').text()
).toBe(
'Application error: a server-side exception has occurred (see the server logs for more information).'
)
expect(
await browser.waitForElementByCss('body').elementByCss('p').text()
).toMatch(/Digest: \w+/)
}
})

if (!isDev) {
it('should allow resetting error boundary', async () => {
const browser = await next.browser('/error/client-component')

// Try triggering and resetting a few times in a row
for (let i = 0; i < 5; i++) {
await browser
.elementByCss('#error-trigger-button')
.click()
.waitForElementByCss('#error-boundary-message')

expect(
await browser.elementByCss('#error-boundary-message').text()
).toBe('An error occurred: this is a test')

await browser
.elementByCss('#reset')
.click()
.waitForElementByCss('#error-trigger-button')

expect(
await browser.elementByCss('#error-trigger-button').text()
).toBe('Trigger Error!')
}
})

it('should hydrate empty shell to handle server-side rendering errors', async () => {
const browser = await next.browser(
'/error/ssr-error-client-component'
)
const logs = await browser.log()
const errors = logs
.filter((x) => x.source === 'error')
.map((x) => x.message)
.join('\n')
expect(errors).toInclude('Error during SSR')
})
}
})

describe('known bugs', () => {
describe('should support React cache', () => {
it('server component', async () => {
Expand Down
9 changes: 9 additions & 0 deletions test/e2e/app-dir/errors/app/layout.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
export default function RootLayout({ children }) {
return (
<html lang="en">
<body>{children}</body>
</html>
)
}

export const revalidate = 0
5 changes: 5 additions & 0 deletions test/e2e/app-dir/errors/app/server-actions/action.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
'use server'

export async function serverAction() {
throw new Error('server action test error')
}
Loading

0 comments on commit 92e4a4b

Please sign in to comment.