From 9a8a2ec6bdb5e795773b3d95132174b2722ff2da Mon Sep 17 00:00:00 2001 From: Jiachi Liu Date: Wed, 26 Jun 2024 17:04:33 +0200 Subject: [PATCH 01/27] test: add test case --- test/e2e/on-request-error/app-basic/app/api/route.js | 5 +++++ .../e2e/on-request-error/app-basic/app/client-page/page.js | 7 +++++++ test/e2e/on-request-error/app-basic/app/layout.js | 7 +++++++ .../e2e/on-request-error/app-basic/app/server-page/page.js | 5 +++++ 4 files changed, 24 insertions(+) create mode 100644 test/e2e/on-request-error/app-basic/app/api/route.js create mode 100644 test/e2e/on-request-error/app-basic/app/client-page/page.js create mode 100644 test/e2e/on-request-error/app-basic/app/layout.js create mode 100644 test/e2e/on-request-error/app-basic/app/server-page/page.js diff --git a/test/e2e/on-request-error/app-basic/app/api/route.js b/test/e2e/on-request-error/app-basic/app/api/route.js new file mode 100644 index 0000000000000..1abf8bfc8d1cd --- /dev/null +++ b/test/e2e/on-request-error/app-basic/app/api/route.js @@ -0,0 +1,5 @@ +export function GET() { + throw new Error('route-node-error') +} + +export const dynamic = 'force-dynamic' diff --git a/test/e2e/on-request-error/app-basic/app/client-page/page.js b/test/e2e/on-request-error/app-basic/app/client-page/page.js new file mode 100644 index 0000000000000..5ce61f4af69b7 --- /dev/null +++ b/test/e2e/on-request-error/app-basic/app/client-page/page.js @@ -0,0 +1,7 @@ +'use client' + +export default function Page() { + throw new Error('client-page-node-error') +} + +export const dynamic = 'force-dynamic' diff --git a/test/e2e/on-request-error/app-basic/app/layout.js b/test/e2e/on-request-error/app-basic/app/layout.js new file mode 100644 index 0000000000000..750eb927b1980 --- /dev/null +++ b/test/e2e/on-request-error/app-basic/app/layout.js @@ -0,0 +1,7 @@ +export default function Layout({ children }) { + return ( + + {children} + + ) +} diff --git a/test/e2e/on-request-error/app-basic/app/server-page/page.js b/test/e2e/on-request-error/app-basic/app/server-page/page.js new file mode 100644 index 0000000000000..4ef362df4225b --- /dev/null +++ b/test/e2e/on-request-error/app-basic/app/server-page/page.js @@ -0,0 +1,5 @@ +export default function Page() { + throw new Error('server-page-node-error') +} + +export const dynamic = 'force-dynamic' From 0c8fe25cb28a751d590f23afe11c6e5b41c96bf0 Mon Sep 17 00:00:00 2001 From: Jiachi Liu Date: Wed, 26 Jun 2024 17:04:53 +0200 Subject: [PATCH 02/27] refactor: load instrumentation module --- .../next/src/server/dev/next-dev-server.ts | 14 +++++++++++--- .../next/src/server/instrumentation/types.ts | 3 +++ packages/next/src/server/next-server.ts | 19 ++++++++++++++----- 3 files changed, 28 insertions(+), 8 deletions(-) create mode 100644 packages/next/src/server/instrumentation/types.ts diff --git a/packages/next/src/server/dev/next-dev-server.ts b/packages/next/src/server/dev/next-dev-server.ts index b975673d72897..a5af77fea5002 100644 --- a/packages/next/src/server/dev/next-dev-server.ts +++ b/packages/next/src/server/dev/next-dev-server.ts @@ -584,7 +584,8 @@ export default class DevServer extends Server { }) } - private async runInstrumentationHookIfAvailable() { + protected async loadInstrumentationModule(): Promise { + let instrumentationModule: any if ( this.actualInstrumentationHookFile && (await this.ensurePage({ @@ -596,15 +597,22 @@ export default class DevServer extends Server { .catch(() => false)) ) { try { - const instrumentationHook = await require( + instrumentationModule = await require( pathJoin(this.distDir, 'server', INSTRUMENTATION_HOOK_FILENAME) ) - await instrumentationHook.register() } catch (err: any) { err.message = `An error occurred while loading instrumentation hook: ${err.message}` throw err } } + return instrumentationModule + } + + private async runInstrumentationHookIfAvailable() { + const instrumentation = await this.loadInstrumentationModule() + if (instrumentation) { + await instrumentation.register() + } } protected async ensureEdgeFunction({ diff --git a/packages/next/src/server/instrumentation/types.ts b/packages/next/src/server/instrumentation/types.ts new file mode 100644 index 0000000000000..e7ca1cb810ba9 --- /dev/null +++ b/packages/next/src/server/instrumentation/types.ts @@ -0,0 +1,3 @@ +export type InstrumentationModule = { + register(): void +} diff --git a/packages/next/src/server/next-server.ts b/packages/next/src/server/next-server.ts index fab8eca0381a8..280067a5a6404 100644 --- a/packages/next/src/server/next-server.ts +++ b/packages/next/src/server/next-server.ts @@ -308,14 +308,14 @@ export default class NextNodeServer extends BaseServer< // development. } - protected async prepareImpl() { - await super.prepareImpl() + protected async loadInstrumentationModule() { + let instrumentationModule: any if ( !this.serverOptions.dev && this.nextConfig.experimental.instrumentationHook ) { try { - const instrumentationHook = await dynamicRequire( + instrumentationModule = await dynamicRequire( resolve( this.serverOptions.dir || '.', this.serverOptions.conf.distDir!, @@ -323,8 +323,6 @@ export default class NextNodeServer extends BaseServer< INSTRUMENTATION_HOOK_FILENAME ) ) - - await instrumentationHook.register?.() } catch (err: any) { if (err.code !== 'MODULE_NOT_FOUND') { err.message = `An error occurred while loading instrumentation hook: ${err.message}` @@ -332,6 +330,17 @@ export default class NextNodeServer extends BaseServer< } } } + return instrumentationModule + } + + protected async prepareImpl() { + await super.prepareImpl() + + // Call the instrumentation register hook + const instrumentation = await this.loadInstrumentationModule() + if (instrumentation) { + await instrumentation.register?.() + } } protected loadEnvConfig({ From 17104bff8eb2c61fb4bd557358e429d5e863bf2f Mon Sep 17 00:00:00 2001 From: Jiachi Liu Date: Tue, 2 Jul 2024 18:23:44 +0200 Subject: [PATCH 03/27] move build exception listeners --- packages/next/src/build/index.ts | 2 +- packages/next/src/{ => build}/lib/setup-exception-listeners.ts | 0 2 files changed, 1 insertion(+), 1 deletion(-) rename packages/next/src/{ => build}/lib/setup-exception-listeners.ts (100%) diff --git a/packages/next/src/build/index.ts b/packages/next/src/build/index.ts index 19c2d388e8327..19363287c1435 100644 --- a/packages/next/src/build/index.ts +++ b/packages/next/src/build/index.ts @@ -6,7 +6,7 @@ import type { ActionManifest } from './webpack/plugins/flight-client-entry-plugi import type { ExportAppOptions } from '../export/types' import type { Revalidate } from '../server/lib/revalidate' -import '../lib/setup-exception-listeners' +import './lib/setup-exception-listeners' import { loadEnvConfig, type LoadedEnvFiles } from '@next/env' import { bold, yellow } from '../lib/picocolors' diff --git a/packages/next/src/lib/setup-exception-listeners.ts b/packages/next/src/build/lib/setup-exception-listeners.ts similarity index 100% rename from packages/next/src/lib/setup-exception-listeners.ts rename to packages/next/src/build/lib/setup-exception-listeners.ts From d73b96935bec745050ec6b1092a00d56fe6c8522 Mon Sep 17 00:00:00 2001 From: Jiachi Liu Date: Wed, 3 Jul 2024 16:04:46 +0200 Subject: [PATCH 04/27] handle nodejs rsc and client server error --- .../next/src/server/app-render/app-render.tsx | 26 ++++++++++++--- .../app-render/create-error-handler.tsx | 8 ++--- packages/next/src/server/app-render/types.ts | 2 +- packages/next/src/server/base-server.ts | 14 ++++++++ .../next/src/server/dev/next-dev-server.ts | 14 ++++++-- packages/next/src/server/next-server.ts | 32 +++++++++++++++---- .../server/route-modules/app-route/module.ts | 25 +++++++++++---- .../app-basic/app/api/edge/route.js | 6 ++++ .../app-basic/app/client-page/edge/page.js | 11 +++++++ .../app-basic/app/client-page/page.js | 5 ++- .../on-request-error/app-basic/app/layout.js | 2 ++ .../app-basic/app/server-page/edge/page.js | 6 ++++ .../app-basic/instrumentation.js | 3 ++ .../on-request-error/app-basic/next.config.js | 5 +++ 14 files changed, 134 insertions(+), 25 deletions(-) create mode 100644 test/e2e/on-request-error/app-basic/app/api/edge/route.js create mode 100644 test/e2e/on-request-error/app-basic/app/client-page/edge/page.js create mode 100644 test/e2e/on-request-error/app-basic/app/server-page/edge/page.js create mode 100644 test/e2e/on-request-error/app-basic/instrumentation.js create mode 100644 test/e2e/on-request-error/app-basic/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 30d8e801b209b..00dd199d2ff62 100644 --- a/packages/next/src/server/app-render/app-render.tsx +++ b/packages/next/src/server/app-render/app-render.tsx @@ -596,7 +596,7 @@ async function renderToHTMLOrFlightImpl( nextFontManifest, supportsDynamicResponse, serverActions, - appDirDevErrorLogger, + onRequestError, assetPrefix = '', enableTainting, } = renderOpts @@ -686,12 +686,30 @@ async function renderToHTMLOrFlightImpl( // intentionally silence the error logger in this case to avoid double // logging. const silenceStaticGenerationErrors = isRoutePPREnabled && isStaticGeneration + const requestContext = { + url: req.url, + method: req.method, + headers: req.headers, + } + + const errorContext = { + routerKind: 'APP_PAGE', + routePath: pagePath, + routeType: 'render', + revalidateReason: '', + renderType: '', + renderSource: '', + } + + function onReactStreamRenderError(err: Error) { + onRequestError?.(err, requestContext, errorContext) + } const serverComponentsErrorHandler = createErrorHandler({ source: ErrorHandlerSource.serverComponents, dev, isNextExport, - errorLogger: appDirDevErrorLogger, + onReactStreamRenderError, digestErrorsMap, silenceLogger: silenceStaticGenerationErrors, }) @@ -699,7 +717,7 @@ async function renderToHTMLOrFlightImpl( source: ErrorHandlerSource.flightData, dev, isNextExport, - errorLogger: appDirDevErrorLogger, + onReactStreamRenderError, digestErrorsMap, silenceLogger: silenceStaticGenerationErrors, }) @@ -707,7 +725,7 @@ async function renderToHTMLOrFlightImpl( source: ErrorHandlerSource.html, dev, isNextExport, - errorLogger: appDirDevErrorLogger, + onReactStreamRenderError, digestErrorsMap, allCapturedErrors, silenceLogger: silenceStaticGenerationErrors, diff --git a/packages/next/src/server/app-render/create-error-handler.tsx b/packages/next/src/server/app-render/create-error-handler.tsx index 008e15299024d..e418cd1046ee2 100644 --- a/packages/next/src/server/app-render/create-error-handler.tsx +++ b/packages/next/src/server/app-render/create-error-handler.tsx @@ -33,7 +33,7 @@ export function createErrorHandler({ source, dev, isNextExport, - errorLogger, + onReactStreamRenderError: onRenderError, digestErrorsMap, allCapturedErrors, silenceLogger, @@ -41,7 +41,7 @@ export function createErrorHandler({ source: (typeof ErrorHandlerSource)[keyof typeof ErrorHandlerSource] dev?: boolean isNextExport?: boolean - errorLogger?: (err: any) => Promise + onReactStreamRenderError?: (err: any) => void // Promise digestErrorsMap: Map allCapturedErrors?: Error[] silenceLogger?: boolean @@ -114,8 +114,8 @@ export function createErrorHandler({ source === 'html') || source === 'flightData' ) { - if (errorLogger) { - errorLogger(err).catch(() => {}) + if (onRenderError) { + onRenderError(err) //.catch(() => {}) } else { // The error logger is currently not provided in the edge runtime. // Use the exposed `__next_log_error__` instead. diff --git a/packages/next/src/server/app-render/types.ts b/packages/next/src/server/app-render/types.ts index d9fded143cd13..4ced854ecc54e 100644 --- a/packages/next/src/server/app-render/types.ts +++ b/packages/next/src/server/app-render/types.ts @@ -142,7 +142,7 @@ export interface RenderOptsPartial { isRevalidate?: boolean nextExport?: boolean nextConfigOutput?: 'standalone' | 'export' - appDirDevErrorLogger?: (err: any) => Promise + onRequestError?: (err: any, request: any, context: any) => void isDraftMode?: boolean deploymentId?: string onUpdateCookies?: (cookies: string[]) => void diff --git a/packages/next/src/server/base-server.ts b/packages/next/src/server/base-server.ts index 2bf501ea02ccf..f65d406a99fc5 100644 --- a/packages/next/src/server/base-server.ts +++ b/packages/next/src/server/base-server.ts @@ -331,6 +331,7 @@ export default abstract class Server< protected readonly clientReferenceManifest?: DeepReadonly protected interceptionRoutePatterns: RegExp[] protected nextFontManifest?: DeepReadonly + protected instrumentation: any private readonly responseCache: ResponseCacheBase protected abstract getPublicDir(): string @@ -574,6 +575,7 @@ export default abstract class Server< clientTraceMetadata: this.nextConfig.experimental.clientTraceMetadata, after: this.nextConfig.experimental.after ?? false, }, + onRequestError: this.instrumentationOnRequestError, } // Initialize next/config with the environment configuration @@ -816,6 +818,16 @@ export default abstract class Server< return matchers } + protected instrumentationOnRequestError = async ( + err: any, + req: any, + context: any + ) => { + if (this.instrumentation) { + this.instrumentation.onRequestError?.(err, req, context) + } + } + public logError(err: Error): void { if (this.quiet) return Log.error(err) @@ -1437,6 +1449,7 @@ export default abstract class Server< throw err } this.logError(getProperError(err)) + console.log('Base server handle request 500', err) res.statusCode = 500 res.body('Internal Server Error').send() } @@ -3358,6 +3371,7 @@ export default abstract class Server< return await this.renderErrorToResponse(ctx, err) } + console.log('Base server render to response 500', err) res.statusCode = 500 // if pages/500 is present we still need to trigger diff --git a/packages/next/src/server/dev/next-dev-server.ts b/packages/next/src/server/dev/next-dev-server.ts index a5af77fea5002..e92d852a89273 100644 --- a/packages/next/src/server/dev/next-dev-server.ts +++ b/packages/next/src/server/dev/next-dev-server.ts @@ -157,8 +157,6 @@ export default class DevServer extends Server { options.startServerSpan ?? trace('start-next-dev-server') this.storeGlobals() this.renderOpts.dev = true - this.renderOpts.appDirDevErrorLogger = (err: any) => - this.logErrorWithOriginalStack(err, 'app-dir') this.renderOpts.ErrorDebug = ReactDevOverlay this.staticPathsCache = new LRUCache({ // 5MB @@ -188,6 +186,12 @@ export default class DevServer extends Server { }) } + this.renderOpts.onRequestError = (err: any, req: any, context: any) => { + super.instrumentationOnRequestError(err, req, context) + // Safe catch to avoid floating promises + this.logErrorWithOriginalStack(err, 'app-dir').catch(() => {}) + } + const { pagesDir, appDir } = findPagesDir(this.dir) this.pagesDir = pagesDir this.appDir = appDir @@ -413,6 +417,7 @@ export default class DevServer extends Server { return { finished: false } } + // Dev middleware error response.statusCode = 500 await this.renderError(err, request, response, parsedUrl.pathname) return { finished: true } @@ -443,6 +448,8 @@ export default class DevServer extends Server { this.logErrorWithOriginalStack(error, 'warning') const err = getProperError(error) const { req, res, page } = params + + // Dev edge function error res.statusCode = 500 await this.renderError(err, req, res, page) return null @@ -506,6 +513,7 @@ export default class DevServer extends Server { return await super.run(req, res, parsedUrl) } catch (error) { const err = getProperError(error) + console.log('dev server err', err) formatServerError(err) this.logErrorWithOriginalStack(err).catch(() => {}) if (!res.sent) { @@ -611,7 +619,7 @@ export default class DevServer extends Server { private async runInstrumentationHookIfAvailable() { const instrumentation = await this.loadInstrumentationModule() if (instrumentation) { - await instrumentation.register() + await instrumentation.register?.() } } diff --git a/packages/next/src/server/next-server.ts b/packages/next/src/server/next-server.ts index 280067a5a6404..1708ab3d01be8 100644 --- a/packages/next/src/server/next-server.ts +++ b/packages/next/src/server/next-server.ts @@ -309,13 +309,13 @@ export default class NextNodeServer extends BaseServer< } protected async loadInstrumentationModule() { - let instrumentationModule: any + // let instrumentationModule: any if ( !this.serverOptions.dev && this.nextConfig.experimental.instrumentationHook ) { try { - instrumentationModule = await dynamicRequire( + this.instrumentation = await dynamicRequire( resolve( this.serverOptions.dir || '.', this.serverOptions.conf.distDir!, @@ -330,16 +330,16 @@ export default class NextNodeServer extends BaseServer< } } } - return instrumentationModule + return this.instrumentation } protected async prepareImpl() { await super.prepareImpl() // Call the instrumentation register hook - const instrumentation = await this.loadInstrumentationModule() - if (instrumentation) { - await instrumentation.register?.() + this.instrumentation = await this.loadInstrumentationModule() + if (this.instrumentation) { + await this.instrumentation.register?.() } } @@ -1734,6 +1734,26 @@ export default class NextNodeServer extends BaseServer< } const error = getProperError(err) + if (this.instrumentation) { + const errorContext = { + routerKind: '', + routePath: '', + routeType: '', + revalidateReason: '', + renderType: '', + renderSource: '', + } + this.instrumentation.onRequestError( + error, + { + url: req.url, + method: req.method, + // cookies: req.headers.cookie, + headers: req.headers, + }, + errorContext + ) + } console.error(error) res.statusCode = 500 await this.renderError(error, req, res, parsed.pathname || '') diff --git a/packages/next/src/server/route-modules/app-route/module.ts b/packages/next/src/server/route-modules/app-route/module.ts index 411df45905b16..9d253b7ed619b 100644 --- a/packages/next/src/server/route-modules/app-route/module.ts +++ b/packages/next/src/server/route-modules/app-route/module.ts @@ -376,16 +376,29 @@ export class AppRouteRouteModule extends RouteModule< this.staticGenerationAsyncStorage, requestAsyncStorage: this.requestAsyncStorage, }) - const res = await handler(request, { - params: context.params - ? parsedUrlQueryToParams(context.params) - : undefined, - }) + let error: unknown + let res + try { + res = await handler(request, { + params: context.params + ? parsedUrlQueryToParams(context.params) + : undefined, + }) + } catch (err) { + error = err + } if (!(res instanceof Response)) { - throw new Error( + error = Error( `No response is returned from route handler '${this.resolvedPagePath}'. Ensure you return a \`Response\` or a \`NextResponse\` in all branches of your handler.` ) } + if (error) { + throw error + } + + // Cast type for TS, if it's not response, it will throw an error above + res = res as Response + context.renderOpts.fetchMetrics = staticGenerationStore.fetchMetrics diff --git a/test/e2e/on-request-error/app-basic/app/api/edge/route.js b/test/e2e/on-request-error/app-basic/app/api/edge/route.js new file mode 100644 index 0000000000000..7d27dac96744a --- /dev/null +++ b/test/e2e/on-request-error/app-basic/app/api/edge/route.js @@ -0,0 +1,6 @@ +export function GET() { + throw new Error('route-edge-error') +} + +export const dynamic = 'force-dynamic' +export const runtime = 'edge' diff --git a/test/e2e/on-request-error/app-basic/app/client-page/edge/page.js b/test/e2e/on-request-error/app-basic/app/client-page/edge/page.js new file mode 100644 index 0000000000000..3438b7bf8b455 --- /dev/null +++ b/test/e2e/on-request-error/app-basic/app/client-page/edge/page.js @@ -0,0 +1,11 @@ +'use client' + +export default function Page() { + if (typeof window === 'undefined') { + throw new Error('client-page-node-error') + } + return
client-page
+} + +export const dynamic = 'force-dynamic' +export const runtime = 'edge' diff --git a/test/e2e/on-request-error/app-basic/app/client-page/page.js b/test/e2e/on-request-error/app-basic/app/client-page/page.js index 5ce61f4af69b7..e1838e3c1c757 100644 --- a/test/e2e/on-request-error/app-basic/app/client-page/page.js +++ b/test/e2e/on-request-error/app-basic/app/client-page/page.js @@ -1,7 +1,10 @@ 'use client' export default function Page() { - throw new Error('client-page-node-error') + if (typeof window === 'undefined') { + throw new Error('client-page-node-error') + } + return
client-page
} export const dynamic = 'force-dynamic' diff --git a/test/e2e/on-request-error/app-basic/app/layout.js b/test/e2e/on-request-error/app-basic/app/layout.js index 750eb927b1980..ff05382bcc3f1 100644 --- a/test/e2e/on-request-error/app-basic/app/layout.js +++ b/test/e2e/on-request-error/app-basic/app/layout.js @@ -5,3 +5,5 @@ export default function Layout({ children }) { ) } + +export const dynamic = 'force-dynamic' diff --git a/test/e2e/on-request-error/app-basic/app/server-page/edge/page.js b/test/e2e/on-request-error/app-basic/app/server-page/edge/page.js new file mode 100644 index 0000000000000..ad14b365d7463 --- /dev/null +++ b/test/e2e/on-request-error/app-basic/app/server-page/edge/page.js @@ -0,0 +1,6 @@ +export default function Page() { + throw new Error('server-page-node-error') +} + +export const dynamic = 'force-dynamic' +export const runtime = 'edge' diff --git a/test/e2e/on-request-error/app-basic/instrumentation.js b/test/e2e/on-request-error/app-basic/instrumentation.js new file mode 100644 index 0000000000000..e3de4c6964347 --- /dev/null +++ b/test/e2e/on-request-error/app-basic/instrumentation.js @@ -0,0 +1,3 @@ +export function onRequestError(err, request, context) { + console.log('[instrumentation]:onRequestError', err, request, context) +} diff --git a/test/e2e/on-request-error/app-basic/next.config.js b/test/e2e/on-request-error/app-basic/next.config.js new file mode 100644 index 0000000000000..c4cf84a76553b --- /dev/null +++ b/test/e2e/on-request-error/app-basic/next.config.js @@ -0,0 +1,5 @@ +module.exports = { + experimental: { + instrumentationHook: true, + }, +} From 8ca280c96465520035deebaa378260daf5cb5d0a Mon Sep 17 00:00:00 2001 From: Jiachi Liu Date: Thu, 4 Jul 2024 14:09:12 +0200 Subject: [PATCH 05/27] cover app custom routes --- packages/next/src/server/base-server.ts | 18 ++++++++++-- .../server/route-modules/app-route/module.ts | 28 ++++++------------- 2 files changed, 24 insertions(+), 22 deletions(-) diff --git a/packages/next/src/server/base-server.ts b/packages/next/src/server/base-server.ts index f65d406a99fc5..72a4faf0db7e5 100644 --- a/packages/next/src/server/base-server.ts +++ b/packages/next/src/server/base-server.ts @@ -575,7 +575,7 @@ export default abstract class Server< clientTraceMetadata: this.nextConfig.experimental.clientTraceMetadata, after: this.nextConfig.experimental.after ?? false, }, - onRequestError: this.instrumentationOnRequestError, + onRequestError: this.instrumentationOnRequestError.bind(this), } // Initialize next/config with the environment configuration @@ -818,11 +818,11 @@ export default abstract class Server< return matchers } - protected instrumentationOnRequestError = async ( + protected async instrumentationOnRequestError( err: any, req: any, context: any - ) => { + ) { if (this.instrumentation) { this.instrumentation.onRequestError?.(err, req, context) } @@ -2407,6 +2407,7 @@ export default abstract class Server< isRevalidate: isSSG, waitUntil: this.getWaitUntil(), onClose: res.onClose.bind(res), + onRequestError: this.renderOpts.onRequestError, }, } @@ -2468,6 +2469,17 @@ export default abstract class Server< // If this is during static generation, throw the error again. if (isSSG) throw err + const errorRequest = { + url: req.url, + method: req.method, + headers: req.headers, + } + const errorContext = { + routePath: '', + } + + renderOpts.onRequestError?.(err, errorRequest, errorContext) + Log.error(err) // Otherwise, send a 500 response. diff --git a/packages/next/src/server/route-modules/app-route/module.ts b/packages/next/src/server/route-modules/app-route/module.ts index 9d253b7ed619b..11a98cb501d9c 100644 --- a/packages/next/src/server/route-modules/app-route/module.ts +++ b/packages/next/src/server/route-modules/app-route/module.ts @@ -56,6 +56,7 @@ import { StaticGenBailoutError } from '../../../client/components/static-generat import { isStaticGenEnabled } from './helpers/is-static-gen-enabled' import { trackDynamicDataAccessed } from '../../app-render/dynamic-rendering' import { ReflectAdapter } from '../../web/spec-extension/adapters/reflect' +import type { RenderOptsPartial } from '../../app-render/types' /** * The AppRouteModule is the type of the module exported by the bundled App @@ -68,7 +69,8 @@ export type AppRouteModule = typeof import('../../../build/templates/app-route') * handler for app routes. */ export interface AppRouteRouteHandlerContext extends RouteModuleHandleContext { - renderOpts: StaticGenerationContext['renderOpts'] + renderOpts: StaticGenerationContext['renderOpts'] & + Pick prerenderManifest: DeepReadonly } @@ -376,28 +378,16 @@ export class AppRouteRouteModule extends RouteModule< this.staticGenerationAsyncStorage, requestAsyncStorage: this.requestAsyncStorage, }) - let error: unknown - let res - try { - res = await handler(request, { - params: context.params - ? parsedUrlQueryToParams(context.params) - : undefined, - }) - } catch (err) { - error = err - } + const res = await handler(request, { + params: context.params + ? parsedUrlQueryToParams(context.params) + : undefined, + }) if (!(res instanceof Response)) { - error = Error( + throw new Error( `No response is returned from route handler '${this.resolvedPagePath}'. Ensure you return a \`Response\` or a \`NextResponse\` in all branches of your handler.` ) } - if (error) { - throw error - } - - // Cast type for TS, if it's not response, it will throw an error above - res = res as Response context.renderOpts.fetchMetrics = staticGenerationStore.fetchMetrics From c6616ab53772c7aaedbb6bbdd1f585a91e9f8842 Mon Sep 17 00:00:00 2001 From: Jiachi Liu Date: Thu, 4 Jul 2024 17:42:18 +0200 Subject: [PATCH 06/27] workaround on handling edge api error --- packages/next/src/server/next-server.ts | 36 +++++++++++++++++++------ 1 file changed, 28 insertions(+), 8 deletions(-) diff --git a/packages/next/src/server/next-server.ts b/packages/next/src/server/next-server.ts index 1708ab3d01be8..b906d07d5d44e 100644 --- a/packages/next/src/server/next-server.ts +++ b/packages/next/src/server/next-server.ts @@ -659,6 +659,7 @@ export default class NextNodeServer extends BaseServer< ctx: RequestContext, bubbleNoFallback: boolean ) { + console.log('renderPageComponent') const edgeFunctionsPages = this.getEdgeFunctionsPages() || [] if (edgeFunctionsPages.length) { const appPaths = this.getOriginalAppPaths(ctx.pathname) @@ -672,14 +673,19 @@ export default class NextNodeServer extends BaseServer< for (const edgeFunctionsPage of edgeFunctionsPages) { if (edgeFunctionsPage === page) { - await this.runEdgeFunction({ - req: ctx.req, - res: ctx.res, - query: ctx.query, - params: ctx.renderOpts.params, - page, - appPaths, - }) + try { + await this.runEdgeFunction({ + req: ctx.req, + res: ctx.res, + query: ctx.query, + params: ctx.renderOpts.params, + page, + appPaths, + }) + } catch (err) { + console.error('renderPageComponent err', err) + throw err + } return null } } @@ -1935,6 +1941,20 @@ export default class NextNodeServer extends BaseServer< incrementalCache: (globalThis as any).__incrementalCache || getRequestMeta(params.req, 'incrementalCache'), + }).catch((err) => { + // For api routes it will directly throw an error, capture the error here + this.renderOpts.onRequestError?.( + err, + { + url: params.req.url, + method: params.req.method, + headers: params.req.headers, + }, + { + routerKind: '', + } + ) + throw err }) if (result.fetchMetrics) { From d603931f33e9f0e1a3c79624944520e69d9884d0 Mon Sep 17 00:00:00 2001 From: Jiachi Liu Date: Fri, 5 Jul 2024 12:00:34 +0200 Subject: [PATCH 07/27] remove try catch --- packages/next/src/server/next-server.ts | 21 ++++++++------------- 1 file changed, 8 insertions(+), 13 deletions(-) diff --git a/packages/next/src/server/next-server.ts b/packages/next/src/server/next-server.ts index b906d07d5d44e..e71382018837f 100644 --- a/packages/next/src/server/next-server.ts +++ b/packages/next/src/server/next-server.ts @@ -673,19 +673,14 @@ export default class NextNodeServer extends BaseServer< for (const edgeFunctionsPage of edgeFunctionsPages) { if (edgeFunctionsPage === page) { - try { - await this.runEdgeFunction({ - req: ctx.req, - res: ctx.res, - query: ctx.query, - params: ctx.renderOpts.params, - page, - appPaths, - }) - } catch (err) { - console.error('renderPageComponent err', err) - throw err - } + await this.runEdgeFunction({ + req: ctx.req, + res: ctx.res, + query: ctx.query, + params: ctx.renderOpts.params, + page, + appPaths, + }) return null } } From 1349425f05b019c7531c9d60108a6fd782ec7e69 Mon Sep 17 00:00:00 2001 From: Jiachi Liu Date: Fri, 5 Jul 2024 16:08:00 +0200 Subject: [PATCH 08/27] edge alias --- .../next/src/build/create-compiler-aliases.ts | 2 +- packages/next/src/server/base-server.ts | 3 +++ packages/next/src/server/next-server.ts | 2 -- packages/next/src/server/web-server.ts | 5 +++++ packages/next/src/server/web/globals.ts | 16 +++++++++++++--- packages/next/types/$$compiled.internal.d.ts | 1 + packages/next/types/compiled.d.ts | 1 + 7 files changed, 24 insertions(+), 6 deletions(-) diff --git a/packages/next/src/build/create-compiler-aliases.ts b/packages/next/src/build/create-compiler-aliases.ts index 03846c9b352fc..628da55ef949c 100644 --- a/packages/next/src/build/create-compiler-aliases.ts +++ b/packages/next/src/build/create-compiler-aliases.ts @@ -54,8 +54,8 @@ export function createWebpackAliases({ // tell webpack where to look for _app and _document // using aliases to allow falling back to the default // version when removed or not present + const nextDistPath = 'next/dist/' + (isEdgeServer ? 'esm/' : '') if (dev) { - const nextDistPath = 'next/dist/' + (isEdgeServer ? 'esm/' : '') customAppAliases[`${PAGES_DIR_ALIAS}/_app`] = [ ...(pagesDir ? pageExtensions.reduce((prev, ext) => { diff --git a/packages/next/src/server/base-server.ts b/packages/next/src/server/base-server.ts index 72a4faf0db7e5..6ae90d686fb52 100644 --- a/packages/next/src/server/base-server.ts +++ b/packages/next/src/server/base-server.ts @@ -1551,6 +1551,8 @@ export default abstract class Server< if (this.prepared) return if (this.preparedPromise === null) { + // Get instrumentation module + this.instrumentation = await this.loadInstrumentationModule() this.preparedPromise = this.prepareImpl().then(() => { this.prepared = true this.preparedPromise = null @@ -1559,6 +1561,7 @@ export default abstract class Server< return this.preparedPromise } protected async prepareImpl(): Promise {} + protected async loadInstrumentationModule(): Promise {} // Backwards compatibility protected async close(): Promise {} diff --git a/packages/next/src/server/next-server.ts b/packages/next/src/server/next-server.ts index e71382018837f..55794fa8757c9 100644 --- a/packages/next/src/server/next-server.ts +++ b/packages/next/src/server/next-server.ts @@ -336,8 +336,6 @@ export default class NextNodeServer extends BaseServer< protected async prepareImpl() { await super.prepareImpl() - // Call the instrumentation register hook - this.instrumentation = await this.loadInstrumentationModule() if (this.instrumentation) { await this.instrumentation.register?.() } diff --git a/packages/next/src/server/web-server.ts b/packages/next/src/server/web-server.ts index f35a602ad3ad3..0ececbad31cf1 100644 --- a/packages/next/src/server/web-server.ts +++ b/packages/next/src/server/web-server.ts @@ -34,6 +34,7 @@ import type { Rewrite } from '../lib/load-custom-routes' import { buildCustomRoute } from '../lib/build-custom-route' import { UNDERSCORE_NOT_FOUND_ROUTE } from '../api/constants' import type { DeepReadonly } from '../shared/lib/deep-readonly' +import { getEdgeInstrumentationModule } from './web/globals' interface WebServerOptions extends Options { webServerConfig: { @@ -413,4 +414,8 @@ export default class NextWebServer extends BaseServer< ) ?? [] ) } + + protected async loadInstrumentationModule() { + return await getEdgeInstrumentationModule() + } } diff --git a/packages/next/src/server/web/globals.ts b/packages/next/src/server/web/globals.ts index 46737d5588f05..1a5c89cc69ce3 100644 --- a/packages/next/src/server/web/globals.ts +++ b/packages/next/src/server/web/globals.ts @@ -1,10 +1,20 @@ declare const _ENTRIES: any -async function registerInstrumentation() { - const register = +export async function getEdgeInstrumentationModule() { + const instrumentation = '_ENTRIES' in globalThis && _ENTRIES.middleware_instrumentation && - (await _ENTRIES.middleware_instrumentation).register + (await _ENTRIES.middleware_instrumentation) + + return instrumentation +} + +let instrumentationModulePromise: Promise | null = null +async function registerInstrumentation() { + if (!instrumentationModulePromise) { + instrumentationModulePromise = getEdgeInstrumentationModule() + } + const { register } = await instrumentationModulePromise if (register) { try { await register() diff --git a/packages/next/types/$$compiled.internal.d.ts b/packages/next/types/$$compiled.internal.d.ts index 18d4b9f961815..d2d701afa30f0 100644 --- a/packages/next/types/$$compiled.internal.d.ts +++ b/packages/next/types/$$compiled.internal.d.ts @@ -32,6 +32,7 @@ declare module 'react-server-dom-webpack/server.node' declare module 'react-server-dom-webpack/client.edge' declare module 'VAR_MODULE_GLOBAL_ERROR' +declare module 'VAR_MODULE_INSTRUMENTATION' declare module 'VAR_USERLAND' declare module 'VAR_MODULE_DOCUMENT' declare module 'VAR_MODULE_APP' diff --git a/packages/next/types/compiled.d.ts b/packages/next/types/compiled.d.ts index 7d6707c1e6571..305507b655f4b 100644 --- a/packages/next/types/compiled.d.ts +++ b/packages/next/types/compiled.d.ts @@ -53,3 +53,4 @@ declare module 'VAR_MODULE_GLOBAL_ERROR' declare module 'VAR_USERLAND' declare module 'VAR_MODULE_DOCUMENT' declare module 'VAR_MODULE_APP' +declare module 'VAR_MODULE_INSTRUMENTATION' From 4000e5c8c630a737fa0faed08f2104830b1ed949 Mon Sep 17 00:00:00 2001 From: Jiachi Liu Date: Sat, 6 Jul 2024 22:09:28 +0200 Subject: [PATCH 09/27] handle edge runtime callback --- .../next/src/server/app-render/app-render.tsx | 11 +++--- .../app-render/create-error-handler.tsx | 2 +- packages/next/src/server/app-render/types.ts | 3 +- packages/next/src/server/base-server.ts | 20 ++++------- .../next/src/server/dev/next-dev-server.ts | 6 +++- .../next/src/server/instrumentation/types.ts | 21 ++++++++++- packages/next/src/server/next-server.ts | 35 ------------------- .../server/route-modules/app-route/module.ts | 2 +- .../app-basic/instrumentation.js | 2 +- 9 files changed, 40 insertions(+), 62 deletions(-) diff --git a/packages/next/src/server/app-render/app-render.tsx b/packages/next/src/server/app-render/app-render.tsx index 00dd199d2ff62..6c3095cc05d0a 100644 --- a/packages/next/src/server/app-render/app-render.tsx +++ b/packages/next/src/server/app-render/app-render.tsx @@ -596,7 +596,7 @@ async function renderToHTMLOrFlightImpl( nextFontManifest, supportsDynamicResponse, serverActions, - onRequestError, + onInstrumentationRequestError, assetPrefix = '', enableTainting, } = renderOpts @@ -693,16 +693,13 @@ async function renderToHTMLOrFlightImpl( } const errorContext = { - routerKind: 'APP_PAGE', + routerKind: 'App Router', routePath: pagePath, routeType: 'render', - revalidateReason: '', - renderType: '', - renderSource: '', - } + } as const function onReactStreamRenderError(err: Error) { - onRequestError?.(err, requestContext, errorContext) + onInstrumentationRequestError?.(err, requestContext, errorContext) } const serverComponentsErrorHandler = createErrorHandler({ diff --git a/packages/next/src/server/app-render/create-error-handler.tsx b/packages/next/src/server/app-render/create-error-handler.tsx index e418cd1046ee2..e4937cb63eafc 100644 --- a/packages/next/src/server/app-render/create-error-handler.tsx +++ b/packages/next/src/server/app-render/create-error-handler.tsx @@ -115,7 +115,7 @@ export function createErrorHandler({ source === 'flightData' ) { if (onRenderError) { - onRenderError(err) //.catch(() => {}) + onRenderError(err) } else { // The error logger is currently not provided in the edge runtime. // Use the exposed `__next_log_error__` instead. diff --git a/packages/next/src/server/app-render/types.ts b/packages/next/src/server/app-render/types.ts index 4ced854ecc54e..0f062338c7c8a 100644 --- a/packages/next/src/server/app-render/types.ts +++ b/packages/next/src/server/app-render/types.ts @@ -11,6 +11,7 @@ import type { DeepReadonly } from '../../shared/lib/deep-readonly' import s from 'next/dist/compiled/superstruct' import type { RequestLifecycleOpts } from '../base-server' +import type { InstrumentationOnRequestError } from '../instrumentation/types' export type DynamicParamTypes = | 'catchall' @@ -142,7 +143,7 @@ export interface RenderOptsPartial { isRevalidate?: boolean nextExport?: boolean nextConfigOutput?: 'standalone' | 'export' - onRequestError?: (err: any, request: any, context: any) => void + onInstrumentationRequestError: InstrumentationOnRequestError isDraftMode?: boolean deploymentId?: string onUpdateCookies?: (cookies: string[]) => void diff --git a/packages/next/src/server/base-server.ts b/packages/next/src/server/base-server.ts index 6ae90d686fb52..9021de5dd90de 100644 --- a/packages/next/src/server/base-server.ts +++ b/packages/next/src/server/base-server.ts @@ -152,6 +152,7 @@ import { type WaitUntil, } from './after/builtin-request-context' import { ENCODED_TAGS } from './stream-utils/encodedTags' +import type { InstrumentationModule } from './instrumentation/types' export type FindComponentsResult = { components: LoadComponentsReturnType @@ -331,7 +332,7 @@ export default abstract class Server< protected readonly clientReferenceManifest?: DeepReadonly protected interceptionRoutePatterns: RegExp[] protected nextFontManifest?: DeepReadonly - protected instrumentation: any + protected instrumentation: InstrumentationModule | undefined private readonly responseCache: ResponseCacheBase protected abstract getPublicDir(): string @@ -575,7 +576,8 @@ export default abstract class Server< clientTraceMetadata: this.nextConfig.experimental.clientTraceMetadata, after: this.nextConfig.experimental.after ?? false, }, - onRequestError: this.instrumentationOnRequestError.bind(this), + onInstrumentationRequestError: + this.instrumentationOnRequestError.bind(this), } // Initialize next/config with the environment configuration @@ -2410,7 +2412,8 @@ export default abstract class Server< isRevalidate: isSSG, waitUntil: this.getWaitUntil(), onClose: res.onClose.bind(res), - onRequestError: this.renderOpts.onRequestError, + onInstrumentationRequestError: + this.renderOpts.onInstrumentationRequestError, }, } @@ -2472,17 +2475,6 @@ export default abstract class Server< // If this is during static generation, throw the error again. if (isSSG) throw err - const errorRequest = { - url: req.url, - method: req.method, - headers: req.headers, - } - const errorContext = { - routePath: '', - } - - renderOpts.onRequestError?.(err, errorRequest, errorContext) - Log.error(err) // Otherwise, send a 500 response. diff --git a/packages/next/src/server/dev/next-dev-server.ts b/packages/next/src/server/dev/next-dev-server.ts index e92d852a89273..e671c79a3e2a3 100644 --- a/packages/next/src/server/dev/next-dev-server.ts +++ b/packages/next/src/server/dev/next-dev-server.ts @@ -19,6 +19,7 @@ import type { UnwrapPromise } from '../../lib/coalesced-function' import type { NodeNextResponse, NodeNextRequest } from '../base-http/node' import type { RouteEnsurer } from '../route-matcher-managers/dev-route-matcher-manager' import type { PagesManifest } from '../../build/webpack/plugins/pages-manifest-plugin' +import type { InstrumentationOnRequestError } from '../instrumentation/types' import fs from 'fs' import { Worker } from 'next/dist/compiled/jest-worker' @@ -186,7 +187,10 @@ export default class DevServer extends Server { }) } - this.renderOpts.onRequestError = (err: any, req: any, context: any) => { + this.renderOpts.onInstrumentationRequestError = ( + ...args: Parameters> + ) => { + const [err, req, context] = args super.instrumentationOnRequestError(err, req, context) // Safe catch to avoid floating promises this.logErrorWithOriginalStack(err, 'app-dir').catch(() => {}) diff --git a/packages/next/src/server/instrumentation/types.ts b/packages/next/src/server/instrumentation/types.ts index e7ca1cb810ba9..a5b7dd263df96 100644 --- a/packages/next/src/server/instrumentation/types.ts +++ b/packages/next/src/server/instrumentation/types.ts @@ -1,3 +1,22 @@ +type RequestErrorContext = { + routerKind: 'Pages Router' | 'App Router' + routePath: string // the route file path, e.g. /app/blog/[dynamic] + routeType: 'render' | 'route' | 'action' | 'middleware' + // TODO: other future instrumentation context +} + export type InstrumentationModule = { - register(): void + register?(): void + onRequestError?( + error: unknown, + errorRequest: Readonly<{ + method: string + url: string + headers: NodeJS.Dict + }>, + errorContext: Readonly + ): void | Promise } + +export type InstrumentationOnRequestError = + InstrumentationModule['onRequestError'] diff --git a/packages/next/src/server/next-server.ts b/packages/next/src/server/next-server.ts index 55794fa8757c9..2acd57a0a1641 100644 --- a/packages/next/src/server/next-server.ts +++ b/packages/next/src/server/next-server.ts @@ -657,7 +657,6 @@ export default class NextNodeServer extends BaseServer< ctx: RequestContext, bubbleNoFallback: boolean ) { - console.log('renderPageComponent') const edgeFunctionsPages = this.getEdgeFunctionsPages() || [] if (edgeFunctionsPages.length) { const appPaths = this.getOriginalAppPaths(ctx.pathname) @@ -1733,26 +1732,6 @@ export default class NextNodeServer extends BaseServer< } const error = getProperError(err) - if (this.instrumentation) { - const errorContext = { - routerKind: '', - routePath: '', - routeType: '', - revalidateReason: '', - renderType: '', - renderSource: '', - } - this.instrumentation.onRequestError( - error, - { - url: req.url, - method: req.method, - // cookies: req.headers.cookie, - headers: req.headers, - }, - errorContext - ) - } console.error(error) res.statusCode = 500 await this.renderError(error, req, res, parsed.pathname || '') @@ -1934,20 +1913,6 @@ export default class NextNodeServer extends BaseServer< incrementalCache: (globalThis as any).__incrementalCache || getRequestMeta(params.req, 'incrementalCache'), - }).catch((err) => { - // For api routes it will directly throw an error, capture the error here - this.renderOpts.onRequestError?.( - err, - { - url: params.req.url, - method: params.req.method, - headers: params.req.headers, - }, - { - routerKind: '', - } - ) - throw err }) if (result.fetchMetrics) { diff --git a/packages/next/src/server/route-modules/app-route/module.ts b/packages/next/src/server/route-modules/app-route/module.ts index 11a98cb501d9c..6ff027da3562c 100644 --- a/packages/next/src/server/route-modules/app-route/module.ts +++ b/packages/next/src/server/route-modules/app-route/module.ts @@ -70,7 +70,7 @@ export type AppRouteModule = typeof import('../../../build/templates/app-route') */ export interface AppRouteRouteHandlerContext extends RouteModuleHandleContext { renderOpts: StaticGenerationContext['renderOpts'] & - Pick + Pick prerenderManifest: DeepReadonly } diff --git a/test/e2e/on-request-error/app-basic/instrumentation.js b/test/e2e/on-request-error/app-basic/instrumentation.js index e3de4c6964347..7d4ee03fb4fbe 100644 --- a/test/e2e/on-request-error/app-basic/instrumentation.js +++ b/test/e2e/on-request-error/app-basic/instrumentation.js @@ -1,3 +1,3 @@ export function onRequestError(err, request, context) { - console.log('[instrumentation]:onRequestError', err, request, context) + console.log('[instrumentation]:onRequestError', err.message, request, context) } From 5cbbcb8527001cc8d5d6cdb0f1ca1e999397a437 Mon Sep 17 00:00:00 2001 From: Jiachi Liu Date: Sat, 6 Jul 2024 23:42:43 +0200 Subject: [PATCH 10/27] handle middleware --- .../next/src/build/templates/middleware.ts | 28 +++++++++- packages/next/src/server/base-server.ts | 13 +++-- .../next/src/server/dev/next-dev-server.ts | 1 - .../next/src/server/instrumentation/types.ts | 23 ++++---- packages/next/src/server/next-server.ts | 54 ++++++++++++++----- .../server/route-modules/app-route/module.ts | 1 - packages/next/src/server/web/globals.ts | 4 +- .../middleware/instrumentation.js | 3 ++ .../on-request-error/middleware/middleware.js | 5 ++ .../middleware/next.config.js | 5 ++ .../on-request-error/middleware/pages/foo.js | 3 ++ .../pages-basic/instrumentation.js | 3 ++ .../pages-basic/next.config.js | 5 ++ .../pages-basic/pages/api/edge.js | 8 +++ .../pages-basic/pages/api/index.js | 7 +++ .../pages-basic/pages/page/edge.js | 8 +++ .../pages-basic/pages/page/index.js | 7 +++ 17 files changed, 141 insertions(+), 37 deletions(-) create mode 100644 test/e2e/on-request-error/middleware/instrumentation.js create mode 100644 test/e2e/on-request-error/middleware/middleware.js create mode 100644 test/e2e/on-request-error/middleware/next.config.js create mode 100644 test/e2e/on-request-error/middleware/pages/foo.js create mode 100644 test/e2e/on-request-error/pages-basic/instrumentation.js create mode 100644 test/e2e/on-request-error/pages-basic/next.config.js create mode 100644 test/e2e/on-request-error/pages-basic/pages/api/edge.js create mode 100644 test/e2e/on-request-error/pages-basic/pages/api/index.js create mode 100644 test/e2e/on-request-error/pages-basic/pages/page/edge.js create mode 100644 test/e2e/on-request-error/pages-basic/pages/page/index.js diff --git a/packages/next/src/build/templates/middleware.ts b/packages/next/src/build/templates/middleware.ts index b143df9006035..3462a3c706fa1 100644 --- a/packages/next/src/build/templates/middleware.ts +++ b/packages/next/src/build/templates/middleware.ts @@ -6,6 +6,7 @@ import { adapter } from '../../server/web/adapter' // Import the userland code. import * as _mod from 'VAR_USERLAND' +import { getEdgeInstrumentationModule } from '../../server/web/globals' const mod = { ..._mod } const handler = mod.middleware || mod.default @@ -18,12 +19,37 @@ if (typeof handler !== 'function') { ) } +function errorHandledHandler(fn: AdapterOptions['handler']) { + return async function (...args: Parameters) { + try { + return await fn(...args) + } catch (error) { + const req = args[0] + const instrumentation = await getEdgeInstrumentationModule() + instrumentation.onRequestError?.( + error, + { + url: req.url, + method: req.method, + headers: Object.fromEntries(req.headers.entries()), + }, + { + routerKind: 'Pages Router', + routePath: '/middleware', + routeType: 'middleware', + } + ) + throw error + } + } +} + export default function nHandler( opts: Omit ) { return adapter({ ...opts, page, - handler, + handler: errorHandledHandler(handler), }) } diff --git a/packages/next/src/server/base-server.ts b/packages/next/src/server/base-server.ts index 9021de5dd90de..9468d64cd98e0 100644 --- a/packages/next/src/server/base-server.ts +++ b/packages/next/src/server/base-server.ts @@ -152,7 +152,10 @@ import { type WaitUntil, } from './after/builtin-request-context' import { ENCODED_TAGS } from './stream-utils/encodedTags' -import type { InstrumentationModule } from './instrumentation/types' +import type { + InstrumentationModule, + InstrumentationOnRequestError, +} from './instrumentation/types' export type FindComponentsResult = { components: LoadComponentsReturnType @@ -821,12 +824,10 @@ export default abstract class Server< } protected async instrumentationOnRequestError( - err: any, - req: any, - context: any + ...args: Parameters ) { if (this.instrumentation) { - this.instrumentation.onRequestError?.(err, req, context) + this.instrumentation.onRequestError?.(...args) } } @@ -1451,7 +1452,6 @@ export default abstract class Server< throw err } this.logError(getProperError(err)) - console.log('Base server handle request 500', err) res.statusCode = 500 res.body('Internal Server Error').send() } @@ -3378,7 +3378,6 @@ export default abstract class Server< return await this.renderErrorToResponse(ctx, err) } - console.log('Base server render to response 500', err) res.statusCode = 500 // if pages/500 is present we still need to trigger diff --git a/packages/next/src/server/dev/next-dev-server.ts b/packages/next/src/server/dev/next-dev-server.ts index e671c79a3e2a3..e3f022012f8f3 100644 --- a/packages/next/src/server/dev/next-dev-server.ts +++ b/packages/next/src/server/dev/next-dev-server.ts @@ -517,7 +517,6 @@ export default class DevServer extends Server { return await super.run(req, res, parsedUrl) } catch (error) { const err = getProperError(error) - console.log('dev server err', err) formatServerError(err) this.logErrorWithOriginalStack(err).catch(() => {}) if (!res.sent) { diff --git a/packages/next/src/server/instrumentation/types.ts b/packages/next/src/server/instrumentation/types.ts index a5b7dd263df96..83587942963e8 100644 --- a/packages/next/src/server/instrumentation/types.ts +++ b/packages/next/src/server/instrumentation/types.ts @@ -5,18 +5,17 @@ type RequestErrorContext = { // TODO: other future instrumentation context } +export type InstrumentationOnRequestError = ( + error: unknown, + errorRequest: Readonly<{ + method: string + url: string + headers: NodeJS.Dict + }>, + errorContext: Readonly +) => void | Promise + export type InstrumentationModule = { register?(): void - onRequestError?( - error: unknown, - errorRequest: Readonly<{ - method: string - url: string - headers: NodeJS.Dict - }>, - errorContext: Readonly - ): void | Promise + onRequestError?: InstrumentationOnRequestError } - -export type InstrumentationOnRequestError = - InstrumentationModule['onRequestError'] diff --git a/packages/next/src/server/next-server.ts b/packages/next/src/server/next-server.ts index 2acd57a0a1641..620a12c7f73af 100644 --- a/packages/next/src/server/next-server.ts +++ b/packages/next/src/server/next-server.ts @@ -976,18 +976,31 @@ export default class NextNodeServer extends BaseServer< delete query._nextBubbleNoFallback delete query[NEXT_RSC_UNION_QUERY] - const handled = await this.runEdgeFunction({ - req, - res, - query, - params: match.params, - page: match.definition.page, - match, - appPaths: null, - }) + try { + const handled = await this.runEdgeFunction({ + req, + res, + query, + params: match.params, + page: match.definition.page, + match, + appPaths: null, + }) - // If we handled the request, we can return early. - if (handled) return true + // If we handled the request, we can return early. + if (handled) return true + } catch (apiError) { + const errorRequest = { + method: req.method, + url: req.url, + headers: req.headers, + } + this.instrumentationOnRequestError(apiError, errorRequest, { + routePath: match.definition.page, + routerKind: 'Pages Router', + routeType: 'route', + } as const) + } } // If the route was detected as being a Pages API route, then handle @@ -1001,8 +1014,21 @@ export default class NextNodeServer extends BaseServer< delete query._nextBubbleNoFallback - const handled = await this.handleApiRequest(req, res, query, match) - if (handled) return true + try { + const handled = await this.handleApiRequest(req, res, query, match) + if (handled) return true + } catch (apiError) { + const errorRequest = { + method: req.method, + url: req.url, + headers: req.headers, + } + this.instrumentationOnRequestError(apiError, errorRequest, { + routePath: match.definition.page, + routerKind: 'Pages Router', + routeType: 'route', + } as const) + } } await this.render(req, res, pathname, query, parsedUrl, true) @@ -1715,7 +1741,7 @@ export default class NextNodeServer extends BaseServer< } return true } - } catch (err: any) { + } catch (err: unknown) { if (bubblingResult) { throw err } diff --git a/packages/next/src/server/route-modules/app-route/module.ts b/packages/next/src/server/route-modules/app-route/module.ts index 6ff027da3562c..2928ca81a227a 100644 --- a/packages/next/src/server/route-modules/app-route/module.ts +++ b/packages/next/src/server/route-modules/app-route/module.ts @@ -388,7 +388,6 @@ export class AppRouteRouteModule extends RouteModule< `No response is returned from route handler '${this.resolvedPagePath}'. Ensure you return a \`Response\` or a \`NextResponse\` in all branches of your handler.` ) } - context.renderOpts.fetchMetrics = staticGenerationStore.fetchMetrics diff --git a/packages/next/src/server/web/globals.ts b/packages/next/src/server/web/globals.ts index 1a5c89cc69ce3..411f39cda8b79 100644 --- a/packages/next/src/server/web/globals.ts +++ b/packages/next/src/server/web/globals.ts @@ -1,6 +1,8 @@ +import type { InstrumentationModule } from '../instrumentation/types' + declare const _ENTRIES: any -export async function getEdgeInstrumentationModule() { +export async function getEdgeInstrumentationModule(): Promise { const instrumentation = '_ENTRIES' in globalThis && _ENTRIES.middleware_instrumentation && diff --git a/test/e2e/on-request-error/middleware/instrumentation.js b/test/e2e/on-request-error/middleware/instrumentation.js new file mode 100644 index 0000000000000..7d4ee03fb4fbe --- /dev/null +++ b/test/e2e/on-request-error/middleware/instrumentation.js @@ -0,0 +1,3 @@ +export function onRequestError(err, request, context) { + console.log('[instrumentation]:onRequestError', err.message, request, context) +} diff --git a/test/e2e/on-request-error/middleware/middleware.js b/test/e2e/on-request-error/middleware/middleware.js new file mode 100644 index 0000000000000..4689fe6dbbee8 --- /dev/null +++ b/test/e2e/on-request-error/middleware/middleware.js @@ -0,0 +1,5 @@ +export function middleware(req) { + if (req.nextUrl.pathname === '/middleware-error') { + throw new Error('Error from middleware') + } +} diff --git a/test/e2e/on-request-error/middleware/next.config.js b/test/e2e/on-request-error/middleware/next.config.js new file mode 100644 index 0000000000000..c4cf84a76553b --- /dev/null +++ b/test/e2e/on-request-error/middleware/next.config.js @@ -0,0 +1,5 @@ +module.exports = { + experimental: { + instrumentationHook: true, + }, +} diff --git a/test/e2e/on-request-error/middleware/pages/foo.js b/test/e2e/on-request-error/middleware/pages/foo.js new file mode 100644 index 0000000000000..fa0f41c9d1702 --- /dev/null +++ b/test/e2e/on-request-error/middleware/pages/foo.js @@ -0,0 +1,3 @@ +export default function Page() { + return 'foo' +} diff --git a/test/e2e/on-request-error/pages-basic/instrumentation.js b/test/e2e/on-request-error/pages-basic/instrumentation.js new file mode 100644 index 0000000000000..7d4ee03fb4fbe --- /dev/null +++ b/test/e2e/on-request-error/pages-basic/instrumentation.js @@ -0,0 +1,3 @@ +export function onRequestError(err, request, context) { + console.log('[instrumentation]:onRequestError', err.message, request, context) +} diff --git a/test/e2e/on-request-error/pages-basic/next.config.js b/test/e2e/on-request-error/pages-basic/next.config.js new file mode 100644 index 0000000000000..c4cf84a76553b --- /dev/null +++ b/test/e2e/on-request-error/pages-basic/next.config.js @@ -0,0 +1,5 @@ +module.exports = { + experimental: { + instrumentationHook: true, + }, +} diff --git a/test/e2e/on-request-error/pages-basic/pages/api/edge.js b/test/e2e/on-request-error/pages-basic/pages/api/edge.js new file mode 100644 index 0000000000000..828e0418d271c --- /dev/null +++ b/test/e2e/on-request-error/pages-basic/pages/api/edge.js @@ -0,0 +1,8 @@ +export default function handle(req, res) { + throw new Error('api-error-edge') +} + +export const config = { + revalidate: 0, + runtime: 'edge', +} diff --git a/test/e2e/on-request-error/pages-basic/pages/api/index.js b/test/e2e/on-request-error/pages-basic/pages/api/index.js new file mode 100644 index 0000000000000..2a37f792217a9 --- /dev/null +++ b/test/e2e/on-request-error/pages-basic/pages/api/index.js @@ -0,0 +1,7 @@ +export default function handle(req, res) { + throw new Error('api-error') +} + +export const config = { + revalidate: 0, +} diff --git a/test/e2e/on-request-error/pages-basic/pages/page/edge.js b/test/e2e/on-request-error/pages-basic/pages/page/edge.js new file mode 100644 index 0000000000000..a67d5868beb3c --- /dev/null +++ b/test/e2e/on-request-error/pages-basic/pages/page/edge.js @@ -0,0 +1,8 @@ +export default function Page() { + throw new Error('pages-error-edge') +} + +export const config = { + revalidate: 0, + runtime: 'experimental-edge', +} diff --git a/test/e2e/on-request-error/pages-basic/pages/page/index.js b/test/e2e/on-request-error/pages-basic/pages/page/index.js new file mode 100644 index 0000000000000..02083dd3d1ad7 --- /dev/null +++ b/test/e2e/on-request-error/pages-basic/pages/page/index.js @@ -0,0 +1,7 @@ +export default function Page() { + throw new Error('pages-error') +} + +export const config = { + revalidate: 0, +} From e8312afcb63c2732063dd4aed472afc96aa83ccd Mon Sep 17 00:00:00 2001 From: Jiachi Liu Date: Sun, 7 Jul 2024 00:40:45 +0200 Subject: [PATCH 11/27] handle pages error --- .../app-render/create-error-handler.tsx | 8 ++-- packages/next/src/server/base-server.ts | 41 +++++++++++++------ 2 files changed, 33 insertions(+), 16 deletions(-) diff --git a/packages/next/src/server/app-render/create-error-handler.tsx b/packages/next/src/server/app-render/create-error-handler.tsx index e4937cb63eafc..6e18d199d3244 100644 --- a/packages/next/src/server/app-render/create-error-handler.tsx +++ b/packages/next/src/server/app-render/create-error-handler.tsx @@ -33,7 +33,7 @@ export function createErrorHandler({ source, dev, isNextExport, - onReactStreamRenderError: onRenderError, + onReactStreamRenderError, digestErrorsMap, allCapturedErrors, silenceLogger, @@ -41,7 +41,7 @@ export function createErrorHandler({ source: (typeof ErrorHandlerSource)[keyof typeof ErrorHandlerSource] dev?: boolean isNextExport?: boolean - onReactStreamRenderError?: (err: any) => void // Promise + onReactStreamRenderError?: (err: any) => void digestErrorsMap: Map allCapturedErrors?: Error[] silenceLogger?: boolean @@ -114,8 +114,8 @@ export function createErrorHandler({ source === 'html') || source === 'flightData' ) { - if (onRenderError) { - onRenderError(err) + if (onReactStreamRenderError) { + onReactStreamRenderError(err) } else { // The error logger is currently not provided in the edge runtime. // Use the exposed `__next_log_error__` instead. diff --git a/packages/next/src/server/base-server.ts b/packages/next/src/server/base-server.ts index 9468d64cd98e0..d753a74d66f55 100644 --- a/packages/next/src/server/base-server.ts +++ b/packages/next/src/server/base-server.ts @@ -2507,18 +2507,35 @@ export default abstract class Server< : res // Call the built-in render method on the module. - result = await routeModule.render( - // TODO: fix this type - // @ts-expect-error - preexisting accepted this - request, - response, - { - page: pathname, - params: opts.params, - query, - renderOpts, - } - ) + try { + result = await routeModule.render( + // TODO: fix this type + // @ts-expect-error - preexisting accepted this + request, + response, + { + page: pathname, + params: opts.params, + query, + renderOpts, + } + ) + } catch (err) { + this.instrumentationOnRequestError( + err, + { + method: req.method, + url: req.url, + headers: req.headers, + }, + { + routerKind: 'Pages Router', + routePath: pathname, + routeType: 'render', + } + ) + throw err + } } else { const module = components.routeModule as AppPageRouteModule From a5d64aff83a7dd6c51dfd99e300da8e9549ca595 Mon Sep 17 00:00:00 2001 From: Jiachi Liu Date: Sun, 7 Jul 2024 18:49:33 +0200 Subject: [PATCH 12/27] add test --- .../app-basic/instrumentation.js | 3 - test/e2e/on-request-error/basic/.gitignore | 1 + .../api => basic/app/app-route}/edge/route.js | 0 .../app/api => basic/app/app-route}/route.js | 0 .../app/client-page/edge/page.js | 2 +- .../app/client-page/page.js | 0 .../{app-basic => basic}/app/layout.js | 0 .../app/server-page/edge/page.js | 2 +- .../app/server-page/page.js | 0 .../basic/app/write-log/route.js | 42 +++++++ test/e2e/on-request-error/basic/basic.test.ts | 114 ++++++++++++++++++ .../on-request-error/basic/instrumentation.js | 13 ++ .../{middleware => basic}/middleware.js | 2 +- .../{app-basic => basic}/next.config.js | 0 .../pages/api/pages-route}/edge.js | 2 +- .../pages/api/pages-route}/index.js | 2 +- .../{pages-basic => basic}/pages/page/edge.js | 2 +- .../pages/page/index.js | 2 +- .../middleware/instrumentation.js | 3 - .../middleware/next.config.js | 5 - .../on-request-error/middleware/pages/foo.js | 3 - .../pages-basic/instrumentation.js | 3 - .../pages-basic/next.config.js | 5 - 23 files changed, 177 insertions(+), 29 deletions(-) delete mode 100644 test/e2e/on-request-error/app-basic/instrumentation.js create mode 100644 test/e2e/on-request-error/basic/.gitignore rename test/e2e/on-request-error/{app-basic/app/api => basic/app/app-route}/edge/route.js (100%) rename test/e2e/on-request-error/{app-basic/app/api => basic/app/app-route}/route.js (100%) rename test/e2e/on-request-error/{app-basic => basic}/app/client-page/edge/page.js (80%) rename test/e2e/on-request-error/{app-basic => basic}/app/client-page/page.js (100%) rename test/e2e/on-request-error/{app-basic => basic}/app/layout.js (100%) rename test/e2e/on-request-error/{app-basic => basic}/app/server-page/edge/page.js (70%) rename test/e2e/on-request-error/{app-basic => basic}/app/server-page/page.js (100%) create mode 100644 test/e2e/on-request-error/basic/app/write-log/route.js create mode 100644 test/e2e/on-request-error/basic/basic.test.ts create mode 100644 test/e2e/on-request-error/basic/instrumentation.js rename test/e2e/on-request-error/{middleware => basic}/middleware.js (67%) rename test/e2e/on-request-error/{app-basic => basic}/next.config.js (100%) rename test/e2e/on-request-error/{pages-basic/pages/api => basic/pages/api/pages-route}/edge.js (75%) rename test/e2e/on-request-error/{pages-basic/pages/api => basic/pages/api/pages-route}/index.js (71%) rename test/e2e/on-request-error/{pages-basic => basic}/pages/page/edge.js (72%) rename test/e2e/on-request-error/{pages-basic => basic}/pages/page/index.js (64%) delete mode 100644 test/e2e/on-request-error/middleware/instrumentation.js delete mode 100644 test/e2e/on-request-error/middleware/next.config.js delete mode 100644 test/e2e/on-request-error/middleware/pages/foo.js delete mode 100644 test/e2e/on-request-error/pages-basic/instrumentation.js delete mode 100644 test/e2e/on-request-error/pages-basic/next.config.js diff --git a/test/e2e/on-request-error/app-basic/instrumentation.js b/test/e2e/on-request-error/app-basic/instrumentation.js deleted file mode 100644 index 7d4ee03fb4fbe..0000000000000 --- a/test/e2e/on-request-error/app-basic/instrumentation.js +++ /dev/null @@ -1,3 +0,0 @@ -export function onRequestError(err, request, context) { - console.log('[instrumentation]:onRequestError', err.message, request, context) -} diff --git a/test/e2e/on-request-error/basic/.gitignore b/test/e2e/on-request-error/basic/.gitignore new file mode 100644 index 0000000000000..1f739ba06a4e6 --- /dev/null +++ b/test/e2e/on-request-error/basic/.gitignore @@ -0,0 +1 @@ +output-log.json diff --git a/test/e2e/on-request-error/app-basic/app/api/edge/route.js b/test/e2e/on-request-error/basic/app/app-route/edge/route.js similarity index 100% rename from test/e2e/on-request-error/app-basic/app/api/edge/route.js rename to test/e2e/on-request-error/basic/app/app-route/edge/route.js diff --git a/test/e2e/on-request-error/app-basic/app/api/route.js b/test/e2e/on-request-error/basic/app/app-route/route.js similarity index 100% rename from test/e2e/on-request-error/app-basic/app/api/route.js rename to test/e2e/on-request-error/basic/app/app-route/route.js diff --git a/test/e2e/on-request-error/app-basic/app/client-page/edge/page.js b/test/e2e/on-request-error/basic/app/client-page/edge/page.js similarity index 80% rename from test/e2e/on-request-error/app-basic/app/client-page/edge/page.js rename to test/e2e/on-request-error/basic/app/client-page/edge/page.js index 3438b7bf8b455..34f27ae5de174 100644 --- a/test/e2e/on-request-error/app-basic/app/client-page/edge/page.js +++ b/test/e2e/on-request-error/basic/app/client-page/edge/page.js @@ -2,7 +2,7 @@ export default function Page() { if (typeof window === 'undefined') { - throw new Error('client-page-node-error') + throw new Error('client-page-edge-error') } return
client-page
} diff --git a/test/e2e/on-request-error/app-basic/app/client-page/page.js b/test/e2e/on-request-error/basic/app/client-page/page.js similarity index 100% rename from test/e2e/on-request-error/app-basic/app/client-page/page.js rename to test/e2e/on-request-error/basic/app/client-page/page.js diff --git a/test/e2e/on-request-error/app-basic/app/layout.js b/test/e2e/on-request-error/basic/app/layout.js similarity index 100% rename from test/e2e/on-request-error/app-basic/app/layout.js rename to test/e2e/on-request-error/basic/app/layout.js diff --git a/test/e2e/on-request-error/app-basic/app/server-page/edge/page.js b/test/e2e/on-request-error/basic/app/server-page/edge/page.js similarity index 70% rename from test/e2e/on-request-error/app-basic/app/server-page/edge/page.js rename to test/e2e/on-request-error/basic/app/server-page/edge/page.js index ad14b365d7463..e42ad40cc2cf5 100644 --- a/test/e2e/on-request-error/app-basic/app/server-page/edge/page.js +++ b/test/e2e/on-request-error/basic/app/server-page/edge/page.js @@ -1,5 +1,5 @@ export default function Page() { - throw new Error('server-page-node-error') + throw new Error('server-page-edge-error') } export const dynamic = 'force-dynamic' diff --git a/test/e2e/on-request-error/app-basic/app/server-page/page.js b/test/e2e/on-request-error/basic/app/server-page/page.js similarity index 100% rename from test/e2e/on-request-error/app-basic/app/server-page/page.js rename to test/e2e/on-request-error/basic/app/server-page/page.js diff --git a/test/e2e/on-request-error/basic/app/write-log/route.js b/test/e2e/on-request-error/basic/app/write-log/route.js new file mode 100644 index 0000000000000..17bf8ba0d8308 --- /dev/null +++ b/test/e2e/on-request-error/basic/app/write-log/route.js @@ -0,0 +1,42 @@ +import fs from 'fs' +import fsp from 'fs/promises' +import path from 'path' + +const dir = path.join(path.dirname(new URL(import.meta.url).pathname), '../..') +const logPath = path.join(dir, 'output-log.json') + +console.log('logPath', logPath) + +export async function POST(req) { + let payloadString = '' + const decoder = new TextDecoder() + const reader = req.clone().body.getReader() + while (true) { + const { done, value } = await reader.read() + if (done) { + break + } + + payloadString += decoder.decode(value) + } + + const payload = JSON.parse(payloadString) + + const json = fs.existsSync(logPath) + ? JSON.parse(await fsp.readFile(logPath, 'utf8')) + : {} + + if (!json[payload.message]) { + json[payload.message] = { + payload, + count: 1, + } + } else { + json[payload.message].count++ + } + + await fsp.writeFile(logPath, JSON.stringify(json, null, 2), 'utf8') + + console.log(`[instrumentation] write-log:${payload.message}`) + return new Response(null, { status: 204 }) +} diff --git a/test/e2e/on-request-error/basic/basic.test.ts b/test/e2e/on-request-error/basic/basic.test.ts new file mode 100644 index 0000000000000..41926d15a6c16 --- /dev/null +++ b/test/e2e/on-request-error/basic/basic.test.ts @@ -0,0 +1,114 @@ +import { nextTestSetup } from 'e2e-utils' +import { retry } from 'next-test-utils' + +describe('on-request-error - basic', () => { + const { next } = nextTestSetup({ + files: __dirname, + }) + + const outputLogPath = 'output-log.json' + + async function getOutputLogJson() { + if (!(await next.hasFile(outputLogPath))) { + return {} + } + const content = await next.readFile(outputLogPath) + return JSON.parse(content) + } + + // let currentLogIndex = 0 + async function validateErrorRecord( + name: string, + url: string, + message?: string + ) { + await retry(async () => { + const currentLog = next.cliOutput //.slice(currentLogIndex) + expect(currentLog).toContain( + `[instrumentation] write-log:${message || name}` + ) + // currentLogIndex = next.cliOutput.length + }) + + const json = await getOutputLogJson() + const record = json[name] + + expect(record).toBeDefined() + const { payload, count } = record + expect(payload.message).toBe(name) + expect(count).toBe(1) + + validateRequestByName(payload.request, url) + } + + function validateRequestByName(request: any, url: string) { + expect(request.url).toBe(url) + expect(request.method).toBe('GET') + expect(request.headers['accept']).toBe('*/*') + } + + beforeAll(async () => { + await next.patchFile(outputLogPath, '{}') + }) + + describe('app router', () => { + it('should catch server component page error in node runtime', async () => { + await next.fetch('/server-page') + await validateErrorRecord('server-page-node-error', '/server-page') + }) + + it('should catch server component page error in edge runtime', async () => { + await next.fetch('/server-page/edge') + await validateErrorRecord('server-page-edge-error', '/server-page/edge') + }) + + it('should catch client component page error in node runtime', async () => { + await next.fetch('/client-page') + await validateErrorRecord('client-page-node-error', '/client-page') + }) + + it('should catch client component page error in edge runtime', async () => { + await next.fetch('/client-page/edge') + await validateErrorRecord('client-page-edge-error', '/client-page/edge') + }) + + it('should catch app routes error in node runtime', async () => { + await next.fetch('/app-route') + await validateErrorRecord('route-node-error', '/app-route') + }) + + it('should catch app routes error in edge runtime', async () => { + await next.fetch('/app-route/edge') + await validateErrorRecord('route-edge-error', '/app-route/edge') + }) + }) + + describe('pages router', () => { + it('should catch pages router page error in node runtime', async () => { + await next.fetch('/page') + await validateErrorRecord('pages-page-node-error', '/page') + }) + + it('should catch pages router page error in edge runtime', async () => { + await next.fetch('/page/edge') + await validateErrorRecord('pages-page-edge-error', '/page/edge') + }) + + it('should catch pages router api error in node runtime', async () => { + await next.fetch('/api/pages-route') + await validateErrorRecord('api-node-error', '/api/pages-route') + }) + + it('should catch pages router api error in edge runtime', async () => { + await next.fetch('/api/pages-route/edge') + await validateErrorRecord('api-edge-error', '/api/pages-route/edge') + }) + }) + + describe('middleware', () => { + it('should catch middleware error', async () => { + await next.fetch('/middleware-error') + await validateErrorRecord('middleware-error', '/middleware-error') + }) + }) +}) diff --git a/test/e2e/on-request-error/basic/instrumentation.js b/test/e2e/on-request-error/basic/instrumentation.js new file mode 100644 index 0000000000000..b9944b351236c --- /dev/null +++ b/test/e2e/on-request-error/basic/instrumentation.js @@ -0,0 +1,13 @@ +export function onRequestError(err, request, context) { + fetch(`http://localhost:${process.env.PORT}/write-log`, { + method: 'POST', + body: JSON.stringify({ + message: err.message, + request, + context, + }), + headers: { + 'Content-Type': 'application/json', + }, + }) +} diff --git a/test/e2e/on-request-error/middleware/middleware.js b/test/e2e/on-request-error/basic/middleware.js similarity index 67% rename from test/e2e/on-request-error/middleware/middleware.js rename to test/e2e/on-request-error/basic/middleware.js index 4689fe6dbbee8..62fc1aff2e47b 100644 --- a/test/e2e/on-request-error/middleware/middleware.js +++ b/test/e2e/on-request-error/basic/middleware.js @@ -1,5 +1,5 @@ export function middleware(req) { if (req.nextUrl.pathname === '/middleware-error') { - throw new Error('Error from middleware') + throw new Error('middleware-error') } } diff --git a/test/e2e/on-request-error/app-basic/next.config.js b/test/e2e/on-request-error/basic/next.config.js similarity index 100% rename from test/e2e/on-request-error/app-basic/next.config.js rename to test/e2e/on-request-error/basic/next.config.js diff --git a/test/e2e/on-request-error/pages-basic/pages/api/edge.js b/test/e2e/on-request-error/basic/pages/api/pages-route/edge.js similarity index 75% rename from test/e2e/on-request-error/pages-basic/pages/api/edge.js rename to test/e2e/on-request-error/basic/pages/api/pages-route/edge.js index 828e0418d271c..f1d3a00e22376 100644 --- a/test/e2e/on-request-error/pages-basic/pages/api/edge.js +++ b/test/e2e/on-request-error/basic/pages/api/pages-route/edge.js @@ -1,5 +1,5 @@ export default function handle(req, res) { - throw new Error('api-error-edge') + throw new Error('api-edge-error') } export const config = { diff --git a/test/e2e/on-request-error/pages-basic/pages/api/index.js b/test/e2e/on-request-error/basic/pages/api/pages-route/index.js similarity index 71% rename from test/e2e/on-request-error/pages-basic/pages/api/index.js rename to test/e2e/on-request-error/basic/pages/api/pages-route/index.js index 2a37f792217a9..50b2183fd4421 100644 --- a/test/e2e/on-request-error/pages-basic/pages/api/index.js +++ b/test/e2e/on-request-error/basic/pages/api/pages-route/index.js @@ -1,5 +1,5 @@ export default function handle(req, res) { - throw new Error('api-error') + throw new Error('api-node-error') } export const config = { diff --git a/test/e2e/on-request-error/pages-basic/pages/page/edge.js b/test/e2e/on-request-error/basic/pages/page/edge.js similarity index 72% rename from test/e2e/on-request-error/pages-basic/pages/page/edge.js rename to test/e2e/on-request-error/basic/pages/page/edge.js index a67d5868beb3c..3f6c6229b5751 100644 --- a/test/e2e/on-request-error/pages-basic/pages/page/edge.js +++ b/test/e2e/on-request-error/basic/pages/page/edge.js @@ -1,5 +1,5 @@ export default function Page() { - throw new Error('pages-error-edge') + throw new Error('pages-page-edge-error') } export const config = { diff --git a/test/e2e/on-request-error/pages-basic/pages/page/index.js b/test/e2e/on-request-error/basic/pages/page/index.js similarity index 64% rename from test/e2e/on-request-error/pages-basic/pages/page/index.js rename to test/e2e/on-request-error/basic/pages/page/index.js index 02083dd3d1ad7..2e5d9530e67c2 100644 --- a/test/e2e/on-request-error/pages-basic/pages/page/index.js +++ b/test/e2e/on-request-error/basic/pages/page/index.js @@ -1,5 +1,5 @@ export default function Page() { - throw new Error('pages-error') + throw new Error('pages-page-node-error') } export const config = { diff --git a/test/e2e/on-request-error/middleware/instrumentation.js b/test/e2e/on-request-error/middleware/instrumentation.js deleted file mode 100644 index 7d4ee03fb4fbe..0000000000000 --- a/test/e2e/on-request-error/middleware/instrumentation.js +++ /dev/null @@ -1,3 +0,0 @@ -export function onRequestError(err, request, context) { - console.log('[instrumentation]:onRequestError', err.message, request, context) -} diff --git a/test/e2e/on-request-error/middleware/next.config.js b/test/e2e/on-request-error/middleware/next.config.js deleted file mode 100644 index c4cf84a76553b..0000000000000 --- a/test/e2e/on-request-error/middleware/next.config.js +++ /dev/null @@ -1,5 +0,0 @@ -module.exports = { - experimental: { - instrumentationHook: true, - }, -} diff --git a/test/e2e/on-request-error/middleware/pages/foo.js b/test/e2e/on-request-error/middleware/pages/foo.js deleted file mode 100644 index fa0f41c9d1702..0000000000000 --- a/test/e2e/on-request-error/middleware/pages/foo.js +++ /dev/null @@ -1,3 +0,0 @@ -export default function Page() { - return 'foo' -} diff --git a/test/e2e/on-request-error/pages-basic/instrumentation.js b/test/e2e/on-request-error/pages-basic/instrumentation.js deleted file mode 100644 index 7d4ee03fb4fbe..0000000000000 --- a/test/e2e/on-request-error/pages-basic/instrumentation.js +++ /dev/null @@ -1,3 +0,0 @@ -export function onRequestError(err, request, context) { - console.log('[instrumentation]:onRequestError', err.message, request, context) -} diff --git a/test/e2e/on-request-error/pages-basic/next.config.js b/test/e2e/on-request-error/pages-basic/next.config.js deleted file mode 100644 index c4cf84a76553b..0000000000000 --- a/test/e2e/on-request-error/pages-basic/next.config.js +++ /dev/null @@ -1,5 +0,0 @@ -module.exports = { - experimental: { - instrumentationHook: true, - }, -} From 76c3b43e1903f9745a59a8b0db805525bf9ac025 Mon Sep 17 00:00:00 2001 From: Jiachi Liu Date: Sun, 7 Jul 2024 21:00:59 +0200 Subject: [PATCH 13/27] handle both dev and prod api in api resolver --- .../src/server/api-utils/node/api-resolver.ts | 18 ++++++- packages/next/src/server/base-server.ts | 14 ++++- .../next/src/server/dev/next-dev-server.ts | 5 +- packages/next/src/server/next-server.ts | 54 +++++-------------- .../server/route-modules/pages-api/module.ts | 9 +++- .../basic/app/write-log/route.js | 2 - test/e2e/on-request-error/basic/basic.test.ts | 33 +++++++----- .../basic/pages/api/pages-route/edge.js | 6 +-- .../basic/pages/api/pages-route/index.js | 4 +- .../on-request-error/basic/pages/page/edge.js | 9 ++-- .../basic/pages/page/index.js | 6 ++- 11 files changed, 88 insertions(+), 72 deletions(-) diff --git a/packages/next/src/server/api-utils/node/api-resolver.ts b/packages/next/src/server/api-utils/node/api-resolver.ts index 85cf47c54e37b..07d69b1ac688c 100644 --- a/packages/next/src/server/api-utils/node/api-resolver.ts +++ b/packages/next/src/server/api-utils/node/api-resolver.ts @@ -29,6 +29,7 @@ import { } from '../../../lib/constants' import { tryGetPreviewData } from './try-get-preview-data' import { parseBody } from './parse-body' +import type { InstrumentationOnRequestError } from '../../instrumentation/types' type RevalidateFn = (config: { urlPath: string @@ -324,7 +325,8 @@ export async function apiResolver( apiContext: ApiContext, propagateError: boolean, dev?: boolean, - page?: string + page?: string, + onRequestError?: InstrumentationOnRequestError ): Promise { const apiReq = req as NextApiRequest const apiRes = res as NextApiResponse @@ -435,6 +437,20 @@ export async function apiResolver( } } } catch (err) { + onRequestError?.( + err, + { + url: req.url || '', + method: req.method || 'GET', + headers: req.headers, + }, + { + routerKind: 'Pages Router', + routePath: page || '', + routeType: 'route', + } + ) + if (err instanceof ApiError) { sendError(apiRes, err.statusCode, err.message) } else { diff --git a/packages/next/src/server/base-server.ts b/packages/next/src/server/base-server.ts index d753a74d66f55..9e2cad1021694 100644 --- a/packages/next/src/server/base-server.ts +++ b/packages/next/src/server/base-server.ts @@ -2475,7 +2475,19 @@ export default abstract class Server< // If this is during static generation, throw the error again. if (isSSG) throw err - Log.error(err) + this.instrumentationOnRequestError( + err, + { + method: req.method, + url: req.url, + headers: req.headers, + }, + { + routerKind: 'App Router', + routePath: pathname, + routeType: 'route', + } + ) // Otherwise, send a 500 response. await sendResponse(req, res, handleInternalServerErrorResponse()) diff --git a/packages/next/src/server/dev/next-dev-server.ts b/packages/next/src/server/dev/next-dev-server.ts index e3f022012f8f3..52ca6b0bc5046 100644 --- a/packages/next/src/server/dev/next-dev-server.ts +++ b/packages/next/src/server/dev/next-dev-server.ts @@ -620,9 +620,8 @@ export default class DevServer extends Server { } private async runInstrumentationHookIfAvailable() { - const instrumentation = await this.loadInstrumentationModule() - if (instrumentation) { - await instrumentation.register?.() + if (this.instrumentation) { + await this.instrumentation.register?.() } } diff --git a/packages/next/src/server/next-server.ts b/packages/next/src/server/next-server.ts index 620a12c7f73af..f6ce42cc4f1ee 100644 --- a/packages/next/src/server/next-server.ts +++ b/packages/next/src/server/next-server.ts @@ -309,7 +309,6 @@ export default class NextNodeServer extends BaseServer< } protected async loadInstrumentationModule() { - // let instrumentationModule: any if ( !this.serverOptions.dev && this.nextConfig.experimental.instrumentationHook @@ -543,6 +542,7 @@ export default class NextNodeServer extends BaseServer< query, params: match.params, page: match.definition.pathname, + onRequestError: this.instrumentation?.onRequestError, }) return true @@ -976,31 +976,18 @@ export default class NextNodeServer extends BaseServer< delete query._nextBubbleNoFallback delete query[NEXT_RSC_UNION_QUERY] - try { - const handled = await this.runEdgeFunction({ - req, - res, - query, - params: match.params, - page: match.definition.page, - match, - appPaths: null, - }) + const handled = await this.runEdgeFunction({ + req, + res, + query, + params: match.params, + page: match.definition.page, + match, + appPaths: null, + }) - // If we handled the request, we can return early. - if (handled) return true - } catch (apiError) { - const errorRequest = { - method: req.method, - url: req.url, - headers: req.headers, - } - this.instrumentationOnRequestError(apiError, errorRequest, { - routePath: match.definition.page, - routerKind: 'Pages Router', - routeType: 'route', - } as const) - } + // If we handled the request, we can return early. + if (handled) return true } // If the route was detected as being a Pages API route, then handle @@ -1014,21 +1001,8 @@ export default class NextNodeServer extends BaseServer< delete query._nextBubbleNoFallback - try { - const handled = await this.handleApiRequest(req, res, query, match) - if (handled) return true - } catch (apiError) { - const errorRequest = { - method: req.method, - url: req.url, - headers: req.headers, - } - this.instrumentationOnRequestError(apiError, errorRequest, { - routePath: match.definition.page, - routerKind: 'Pages Router', - routeType: 'route', - } as const) - } + const handled = await this.handleApiRequest(req, res, query, match) + if (handled) return true } await this.render(req, res, pathname, query, parsedUrl, true) diff --git a/packages/next/src/server/route-modules/pages-api/module.ts b/packages/next/src/server/route-modules/pages-api/module.ts index ac8d2bea98003..c2c79e1fa7564 100644 --- a/packages/next/src/server/route-modules/pages-api/module.ts +++ b/packages/next/src/server/route-modules/pages-api/module.ts @@ -7,6 +7,7 @@ import type { RouteModuleOptions } from '../route-module' import { RouteModule, type RouteModuleHandleContext } from '../route-module' import { apiResolver } from '../../api-utils/node/api-resolver' +import type { InstrumentationOnRequestError } from '../../instrumentation/types' type PagesAPIHandleFn = ( req: IncomingMessage, @@ -92,6 +93,11 @@ type PagesAPIRouteHandlerContext = RouteModuleHandleContext & { * The page that's being rendered. */ page: string + + /** + * The error handler for the request. + */ + onRequestError?: InstrumentationOnRequestError } export type PagesAPIRouteModuleOptions = RouteModuleOptions< @@ -146,7 +152,8 @@ export class PagesAPIRouteModule extends RouteModule< }, context.minimalMode, context.dev, - context.page + context.page, + context.onRequestError ) } } diff --git a/test/e2e/on-request-error/basic/app/write-log/route.js b/test/e2e/on-request-error/basic/app/write-log/route.js index 17bf8ba0d8308..62b11b6617cd8 100644 --- a/test/e2e/on-request-error/basic/app/write-log/route.js +++ b/test/e2e/on-request-error/basic/app/write-log/route.js @@ -5,8 +5,6 @@ import path from 'path' const dir = path.join(path.dirname(new URL(import.meta.url).pathname), '../..') const logPath = path.join(dir, 'output-log.json') -console.log('logPath', logPath) - export async function POST(req) { let payloadString = '' const decoder = new TextDecoder() diff --git a/test/e2e/on-request-error/basic/basic.test.ts b/test/e2e/on-request-error/basic/basic.test.ts index 41926d15a6c16..84dc65065387d 100644 --- a/test/e2e/on-request-error/basic/basic.test.ts +++ b/test/e2e/on-request-error/basic/basic.test.ts @@ -16,19 +16,18 @@ describe('on-request-error - basic', () => { return JSON.parse(content) } - // let currentLogIndex = 0 async function validateErrorRecord( name: string, url: string, - message?: string + isMiddleware: boolean = false ) { await retry(async () => { - const currentLog = next.cliOutput //.slice(currentLogIndex) - expect(currentLog).toContain( - `[instrumentation] write-log:${message || name}` - ) - // currentLogIndex = next.cliOutput.length - }) + const recordLogs = next.cliOutput + .split('\n') + .filter((log) => log.includes('[instrumentation] write-log')) + const expectedLog = recordLogs.find((log) => log.includes(name)) + expect(expectedLog).toBeDefined() + }, 5000) const json = await getOutputLogJson() const record = json[name] @@ -38,11 +37,21 @@ describe('on-request-error - basic', () => { expect(payload.message).toBe(name) expect(count).toBe(1) - validateRequestByName(payload.request, url) + validateRequestByName(payload.request, url, isMiddleware) } - function validateRequestByName(request: any, url: string) { - expect(request.url).toBe(url) + function validateRequestByName( + request: any, + url: string, + isMiddleware: boolean = false + ) { + if (isMiddleware) { + // For middleware, the URL is absolute url with host + expect(request.url).toMatch(/^http:\/\//) + expect(request.url).toMatch(url) + } else { + expect(request.url).toBe(url) + } expect(request.method).toBe('GET') expect(request.headers['accept']).toBe('*/*') } @@ -108,7 +117,7 @@ describe('on-request-error - basic', () => { describe('middleware', () => { it('should catch middleware error', async () => { await next.fetch('/middleware-error') - await validateErrorRecord('middleware-error', '/middleware-error') + await validateErrorRecord('middleware-error', '/middleware-error', true) }) }) }) diff --git a/test/e2e/on-request-error/basic/pages/api/pages-route/edge.js b/test/e2e/on-request-error/basic/pages/api/pages-route/edge.js index f1d3a00e22376..9e962b0d8cb49 100644 --- a/test/e2e/on-request-error/basic/pages/api/pages-route/edge.js +++ b/test/e2e/on-request-error/basic/pages/api/pages-route/edge.js @@ -2,7 +2,5 @@ export default function handle(req, res) { throw new Error('api-edge-error') } -export const config = { - revalidate: 0, - runtime: 'edge', -} +export const runtime = 'edge' +export const revalidate = 0 diff --git a/test/e2e/on-request-error/basic/pages/api/pages-route/index.js b/test/e2e/on-request-error/basic/pages/api/pages-route/index.js index 50b2183fd4421..224ff69919852 100644 --- a/test/e2e/on-request-error/basic/pages/api/pages-route/index.js +++ b/test/e2e/on-request-error/basic/pages/api/pages-route/index.js @@ -2,6 +2,4 @@ export default function handle(req, res) { throw new Error('api-node-error') } -export const config = { - revalidate: 0, -} +export const revalidate = 0 diff --git a/test/e2e/on-request-error/basic/pages/page/edge.js b/test/e2e/on-request-error/basic/pages/page/edge.js index 3f6c6229b5751..747cf6a1163ff 100644 --- a/test/e2e/on-request-error/basic/pages/page/edge.js +++ b/test/e2e/on-request-error/basic/pages/page/edge.js @@ -2,7 +2,10 @@ export default function Page() { throw new Error('pages-page-edge-error') } -export const config = { - revalidate: 0, - runtime: 'experimental-edge', +export async function getServerSideProps() { + return { + props: {}, + } } + +export const runtime = 'experimental-edge' diff --git a/test/e2e/on-request-error/basic/pages/page/index.js b/test/e2e/on-request-error/basic/pages/page/index.js index 2e5d9530e67c2..915103f149d21 100644 --- a/test/e2e/on-request-error/basic/pages/page/index.js +++ b/test/e2e/on-request-error/basic/pages/page/index.js @@ -2,6 +2,8 @@ export default function Page() { throw new Error('pages-page-node-error') } -export const config = { - revalidate: 0, +export async function getServerSideProps() { + return { + props: {}, + } } From d8370cdc2603a86197e636d02f3cc937179e5dcb Mon Sep 17 00:00:00 2001 From: Jiachi Liu Date: Sun, 7 Jul 2024 21:32:57 +0200 Subject: [PATCH 14/27] normalize req in one place --- .../next/src/build/create-compiler-aliases.ts | 2 +- packages/next/src/build/index.ts | 2 +- .../next/src/build/templates/middleware.ts | 2 + .../lib/setup-exception-listeners.ts | 0 .../src/server/api-utils/node/api-resolver.ts | 22 ++----- .../next/src/server/app-render/app-render.tsx | 7 +- packages/next/src/server/app-render/types.ts | 13 +++- packages/next/src/server/base-server.ts | 64 +++++++++---------- .../next/src/server/dev/next-dev-server.ts | 5 +- packages/next/src/server/next-server.ts | 2 +- .../server/route-modules/pages-api/module.ts | 5 +- 11 files changed, 59 insertions(+), 65 deletions(-) rename packages/next/src/{build => }/lib/setup-exception-listeners.ts (100%) diff --git a/packages/next/src/build/create-compiler-aliases.ts b/packages/next/src/build/create-compiler-aliases.ts index 628da55ef949c..03846c9b352fc 100644 --- a/packages/next/src/build/create-compiler-aliases.ts +++ b/packages/next/src/build/create-compiler-aliases.ts @@ -54,8 +54,8 @@ export function createWebpackAliases({ // tell webpack where to look for _app and _document // using aliases to allow falling back to the default // version when removed or not present - const nextDistPath = 'next/dist/' + (isEdgeServer ? 'esm/' : '') if (dev) { + const nextDistPath = 'next/dist/' + (isEdgeServer ? 'esm/' : '') customAppAliases[`${PAGES_DIR_ALIAS}/_app`] = [ ...(pagesDir ? pageExtensions.reduce((prev, ext) => { diff --git a/packages/next/src/build/index.ts b/packages/next/src/build/index.ts index 19363287c1435..19c2d388e8327 100644 --- a/packages/next/src/build/index.ts +++ b/packages/next/src/build/index.ts @@ -6,7 +6,7 @@ import type { ActionManifest } from './webpack/plugins/flight-client-entry-plugi import type { ExportAppOptions } from '../export/types' import type { Revalidate } from '../server/lib/revalidate' -import './lib/setup-exception-listeners' +import '../lib/setup-exception-listeners' import { loadEnvConfig, type LoadedEnvFiles } from '@next/env' import { bold, yellow } from '../lib/picocolors' diff --git a/packages/next/src/build/templates/middleware.ts b/packages/next/src/build/templates/middleware.ts index 3462a3c706fa1..ff1a6f8e45c7d 100644 --- a/packages/next/src/build/templates/middleware.ts +++ b/packages/next/src/build/templates/middleware.ts @@ -19,6 +19,8 @@ if (typeof handler !== 'function') { ) } +// Middleware will only sent out the FetchEvent to next server, +// so load instrumentation module here and track the error inside middleware module. function errorHandledHandler(fn: AdapterOptions['handler']) { return async function (...args: Parameters) { try { diff --git a/packages/next/src/build/lib/setup-exception-listeners.ts b/packages/next/src/lib/setup-exception-listeners.ts similarity index 100% rename from packages/next/src/build/lib/setup-exception-listeners.ts rename to packages/next/src/lib/setup-exception-listeners.ts diff --git a/packages/next/src/server/api-utils/node/api-resolver.ts b/packages/next/src/server/api-utils/node/api-resolver.ts index 07d69b1ac688c..8421af9eac356 100644 --- a/packages/next/src/server/api-utils/node/api-resolver.ts +++ b/packages/next/src/server/api-utils/node/api-resolver.ts @@ -3,6 +3,7 @@ import type { NextApiRequest, NextApiResponse } from '../../../shared/lib/utils' import type { PageConfig, ResponseLimit } from '../../../types' import type { __ApiPreviewProps } from '../.' import type { CookieSerializeOptions } from 'next/dist/compiled/cookie' +import type { ServerOnInstrumentationRequestError } from '../../app-render/types' import bytes from 'next/dist/compiled/bytes' import { generateETag } from '../../lib/etag' @@ -29,7 +30,6 @@ import { } from '../../../lib/constants' import { tryGetPreviewData } from './try-get-preview-data' import { parseBody } from './parse-body' -import type { InstrumentationOnRequestError } from '../../instrumentation/types' type RevalidateFn = (config: { urlPath: string @@ -326,7 +326,7 @@ export async function apiResolver( propagateError: boolean, dev?: boolean, page?: string, - onRequestError?: InstrumentationOnRequestError + onError?: ServerOnInstrumentationRequestError ): Promise { const apiReq = req as NextApiRequest const apiRes = res as NextApiResponse @@ -437,19 +437,11 @@ export async function apiResolver( } } } catch (err) { - onRequestError?.( - err, - { - url: req.url || '', - method: req.method || 'GET', - headers: req.headers, - }, - { - routerKind: 'Pages Router', - routePath: page || '', - routeType: 'route', - } - ) + onError?.(err, req, { + routerKind: 'Pages Router', + routePath: page || '', + routeType: 'route', + }) if (err instanceof ApiError) { sendError(apiRes, err.statusCode, err.message) diff --git a/packages/next/src/server/app-render/app-render.tsx b/packages/next/src/server/app-render/app-render.tsx index 6c3095cc05d0a..bc32845567caa 100644 --- a/packages/next/src/server/app-render/app-render.tsx +++ b/packages/next/src/server/app-render/app-render.tsx @@ -686,11 +686,6 @@ async function renderToHTMLOrFlightImpl( // intentionally silence the error logger in this case to avoid double // logging. const silenceStaticGenerationErrors = isRoutePPREnabled && isStaticGeneration - const requestContext = { - url: req.url, - method: req.method, - headers: req.headers, - } const errorContext = { routerKind: 'App Router', @@ -699,7 +694,7 @@ async function renderToHTMLOrFlightImpl( } as const function onReactStreamRenderError(err: Error) { - onInstrumentationRequestError?.(err, requestContext, errorContext) + onInstrumentationRequestError?.(err, req, errorContext) } const serverComponentsErrorHandler = createErrorHandler({ diff --git a/packages/next/src/server/app-render/types.ts b/packages/next/src/server/app-render/types.ts index 0f062338c7c8a..764afee96c7f5 100644 --- a/packages/next/src/server/app-render/types.ts +++ b/packages/next/src/server/app-render/types.ts @@ -12,6 +12,9 @@ import type { DeepReadonly } from '../../shared/lib/deep-readonly' import s from 'next/dist/compiled/superstruct' import type { RequestLifecycleOpts } from '../base-server' import type { InstrumentationOnRequestError } from '../instrumentation/types' +import type { NextRequestHint } from '../web/adapter' +import type { BaseNextRequest } from '../base-http' +import type { IncomingMessage } from 'http' export type DynamicParamTypes = | 'catchall' @@ -124,6 +127,14 @@ export type ActionFlightResponse = // This case happens when `redirect()` is called in a server action. | NextFlightResponse +export type ServerOnInstrumentationRequestError = ( + error: unknown, + // The request could be middleware, node server or web server request, + // we normalized them into an aligned format to `onRequestError` API later. + request: NextRequestHint | BaseNextRequest | IncomingMessage, + errorContext: Parameters[2] +) => void | Promise + export interface RenderOptsPartial { err?: Error | null dev?: boolean @@ -143,7 +154,7 @@ export interface RenderOptsPartial { isRevalidate?: boolean nextExport?: boolean nextConfigOutput?: 'standalone' | 'export' - onInstrumentationRequestError: InstrumentationOnRequestError + onInstrumentationRequestError?: ServerOnInstrumentationRequestError isDraftMode?: boolean deploymentId?: string onUpdateCookies?: (cookies: string[]) => void diff --git a/packages/next/src/server/base-server.ts b/packages/next/src/server/base-server.ts index 9e2cad1021694..63af3c3212af7 100644 --- a/packages/next/src/server/base-server.ts +++ b/packages/next/src/server/base-server.ts @@ -11,7 +11,10 @@ import type { } from './request-meta' import type { ParsedUrlQuery } from 'querystring' import type { RenderOptsPartial as PagesRenderOptsPartial } from './render' -import type { RenderOptsPartial as AppRenderOptsPartial } from './app-render/types' +import type { + RenderOptsPartial as AppRenderOptsPartial, + ServerOnInstrumentationRequestError, +} from './app-render/types' import type { CachedAppPageValue, CachedPageValue, @@ -47,6 +50,7 @@ import type { import type { MiddlewareMatcher } from '../build/analysis/get-page-static-info' import type { TLSSocket } from 'tls' import type { PathnameNormalizer } from './normalizers/request/pathname-normalizer' +import type { InstrumentationModule } from './instrumentation/types' import { format as formatUrl, parse as parseUrl } from 'url' import { formatHostname } from './lib/format-hostname' @@ -152,10 +156,7 @@ import { type WaitUntil, } from './after/builtin-request-context' import { ENCODED_TAGS } from './stream-utils/encodedTags' -import type { - InstrumentationModule, - InstrumentationOnRequestError, -} from './instrumentation/types' +import { NextRequestHint } from './web/adapter' export type FindComponentsResult = { components: LoadComponentsReturnType @@ -824,10 +825,23 @@ export default abstract class Server< } protected async instrumentationOnRequestError( - ...args: Parameters + ...args: Parameters ) { + const [err, req, ctx] = args if (this.instrumentation) { - this.instrumentation.onRequestError?.(...args) + this.instrumentation.onRequestError?.( + err, + { + url: req.url || '', + method: req.method || 'GET', + // Normalize middleware headers and other server request headers + headers: + req instanceof NextRequestHint + ? Object.fromEntries(req.headers.entries()) + : req.headers, + }, + ctx + ) } } @@ -2475,19 +2489,11 @@ export default abstract class Server< // If this is during static generation, throw the error again. if (isSSG) throw err - this.instrumentationOnRequestError( - err, - { - method: req.method, - url: req.url, - headers: req.headers, - }, - { - routerKind: 'App Router', - routePath: pathname, - routeType: 'route', - } - ) + this.instrumentationOnRequestError(err, req, { + routerKind: 'App Router', + routePath: pathname, + routeType: 'route', + }) // Otherwise, send a 500 response. await sendResponse(req, res, handleInternalServerErrorResponse()) @@ -2533,19 +2539,11 @@ export default abstract class Server< } ) } catch (err) { - this.instrumentationOnRequestError( - err, - { - method: req.method, - url: req.url, - headers: req.headers, - }, - { - routerKind: 'Pages Router', - routePath: pathname, - routeType: 'render', - } - ) + this.instrumentationOnRequestError(err, req, { + routerKind: 'Pages Router', + routePath: pathname, + routeType: 'render', + }) throw err } } else { diff --git a/packages/next/src/server/dev/next-dev-server.ts b/packages/next/src/server/dev/next-dev-server.ts index 52ca6b0bc5046..c7ac4aaf16bcf 100644 --- a/packages/next/src/server/dev/next-dev-server.ts +++ b/packages/next/src/server/dev/next-dev-server.ts @@ -19,7 +19,6 @@ import type { UnwrapPromise } from '../../lib/coalesced-function' import type { NodeNextResponse, NodeNextRequest } from '../base-http/node' import type { RouteEnsurer } from '../route-matcher-managers/dev-route-matcher-manager' import type { PagesManifest } from '../../build/webpack/plugins/pages-manifest-plugin' -import type { InstrumentationOnRequestError } from '../instrumentation/types' import fs from 'fs' import { Worker } from 'next/dist/compiled/jest-worker' @@ -187,9 +186,7 @@ export default class DevServer extends Server { }) } - this.renderOpts.onInstrumentationRequestError = ( - ...args: Parameters> - ) => { + this.renderOpts.onInstrumentationRequestError = (...args) => { const [err, req, context] = args super.instrumentationOnRequestError(err, req, context) // Safe catch to avoid floating promises diff --git a/packages/next/src/server/next-server.ts b/packages/next/src/server/next-server.ts index f6ce42cc4f1ee..62eacd54a43c1 100644 --- a/packages/next/src/server/next-server.ts +++ b/packages/next/src/server/next-server.ts @@ -542,7 +542,7 @@ export default class NextNodeServer extends BaseServer< query, params: match.params, page: match.definition.pathname, - onRequestError: this.instrumentation?.onRequestError, + onError: this.instrumentationOnRequestError, }) return true diff --git a/packages/next/src/server/route-modules/pages-api/module.ts b/packages/next/src/server/route-modules/pages-api/module.ts index c2c79e1fa7564..6eacf264488ff 100644 --- a/packages/next/src/server/route-modules/pages-api/module.ts +++ b/packages/next/src/server/route-modules/pages-api/module.ts @@ -7,7 +7,6 @@ import type { RouteModuleOptions } from '../route-module' import { RouteModule, type RouteModuleHandleContext } from '../route-module' import { apiResolver } from '../../api-utils/node/api-resolver' -import type { InstrumentationOnRequestError } from '../../instrumentation/types' type PagesAPIHandleFn = ( req: IncomingMessage, @@ -97,7 +96,7 @@ type PagesAPIRouteHandlerContext = RouteModuleHandleContext & { /** * The error handler for the request. */ - onRequestError?: InstrumentationOnRequestError + onError?: Parameters[8] } export type PagesAPIRouteModuleOptions = RouteModuleOptions< @@ -153,7 +152,7 @@ export class PagesAPIRouteModule extends RouteModule< context.minimalMode, context.dev, context.page, - context.onRequestError + context.onError ) } } From 7b90c9e13b46df45e525bddf27167d210a97d079 Mon Sep 17 00:00:00 2001 From: Jiachi Liu Date: Mon, 8 Jul 2024 01:10:52 +0200 Subject: [PATCH 15/27] fix middleware and edge functions log --- packages/next/src/build/templates/middleware.ts | 2 +- .../server/app-render/create-error-handler.tsx | 17 ++++++++--------- packages/next/src/server/base-server.ts | 3 +++ packages/next/src/server/dev/next-dev-server.ts | 2 -- packages/next/src/server/web/globals.ts | 10 ++++++---- 5 files changed, 18 insertions(+), 16 deletions(-) diff --git a/packages/next/src/build/templates/middleware.ts b/packages/next/src/build/templates/middleware.ts index ff1a6f8e45c7d..7dfffc780ec10 100644 --- a/packages/next/src/build/templates/middleware.ts +++ b/packages/next/src/build/templates/middleware.ts @@ -28,7 +28,7 @@ function errorHandledHandler(fn: AdapterOptions['handler']) { } catch (error) { const req = args[0] const instrumentation = await getEdgeInstrumentationModule() - instrumentation.onRequestError?.( + instrumentation?.onRequestError?.( error, { url: req.url, diff --git a/packages/next/src/server/app-render/create-error-handler.tsx b/packages/next/src/server/app-render/create-error-handler.tsx index 6e18d199d3244..83bbd982590b6 100644 --- a/packages/next/src/server/app-render/create-error-handler.tsx +++ b/packages/next/src/server/app-render/create-error-handler.tsx @@ -114,17 +114,16 @@ export function createErrorHandler({ source === 'html') || source === 'flightData' ) { - if (onReactStreamRenderError) { + // Dev mode logging: + // The error logger is currently not provided in the edge runtime. + // Use the exposed `__next_log_error__` instead. + // This will trace error traces to the original source code. + if (typeof __next_log_error__ === 'function') { + __next_log_error__(err) + } else if (onReactStreamRenderError) { onReactStreamRenderError(err) } else { - // The error logger is currently not provided in the edge runtime. - // Use the exposed `__next_log_error__` instead. - // This will trace error traces to the original source code. - if (typeof __next_log_error__ === 'function') { - __next_log_error__(err) - } else { - console.error(err) - } + console.error(err) } } } diff --git a/packages/next/src/server/base-server.ts b/packages/next/src/server/base-server.ts index 63af3c3212af7..4dc0904f56401 100644 --- a/packages/next/src/server/base-server.ts +++ b/packages/next/src/server/base-server.ts @@ -828,6 +828,7 @@ export default abstract class Server< ...args: Parameters ) { const [err, req, ctx] = args + if (this.instrumentation) { this.instrumentation.onRequestError?.( err, @@ -2489,6 +2490,8 @@ export default abstract class Server< // If this is during static generation, throw the error again. if (isSSG) throw err + Log.error(err) + this.instrumentationOnRequestError(err, req, { routerKind: 'App Router', routePath: pathname, diff --git a/packages/next/src/server/dev/next-dev-server.ts b/packages/next/src/server/dev/next-dev-server.ts index c7ac4aaf16bcf..315ac3f9d6c61 100644 --- a/packages/next/src/server/dev/next-dev-server.ts +++ b/packages/next/src/server/dev/next-dev-server.ts @@ -418,7 +418,6 @@ export default class DevServer extends Server { return { finished: false } } - // Dev middleware error response.statusCode = 500 await this.renderError(err, request, response, parsedUrl.pathname) return { finished: true } @@ -450,7 +449,6 @@ export default class DevServer extends Server { const err = getProperError(error) const { req, res, page } = params - // Dev edge function error res.statusCode = 500 await this.renderError(err, req, res, page) return null diff --git a/packages/next/src/server/web/globals.ts b/packages/next/src/server/web/globals.ts index 411f39cda8b79..500f5cdf6ab26 100644 --- a/packages/next/src/server/web/globals.ts +++ b/packages/next/src/server/web/globals.ts @@ -2,7 +2,9 @@ import type { InstrumentationModule } from '../instrumentation/types' declare const _ENTRIES: any -export async function getEdgeInstrumentationModule(): Promise { +export async function getEdgeInstrumentationModule(): Promise< + InstrumentationModule | undefined +> { const instrumentation = '_ENTRIES' in globalThis && _ENTRIES.middleware_instrumentation && @@ -16,10 +18,10 @@ async function registerInstrumentation() { if (!instrumentationModulePromise) { instrumentationModulePromise = getEdgeInstrumentationModule() } - const { register } = await instrumentationModulePromise - if (register) { + const instrumentation = await instrumentationModulePromise + if (instrumentation?.register) { try { - await register() + await instrumentation.register() } catch (err: any) { err.message = `An error occurred while loading instrumentation hook: ${err.message}` throw err From aadad8d53370a03d8b0ae873a939134cbcfe829d Mon Sep 17 00:00:00 2001 From: Jiachi Liu Date: Mon, 8 Jul 2024 13:02:46 +0200 Subject: [PATCH 16/27] fix pages api route handling --- .../app-render/create-error-handler.tsx | 4 +-- packages/next/src/server/next-server.ts | 33 ++++++++++++------- 2 files changed, 22 insertions(+), 15 deletions(-) diff --git a/packages/next/src/server/app-render/create-error-handler.tsx b/packages/next/src/server/app-render/create-error-handler.tsx index 83bbd982590b6..c8ba43f8959d2 100644 --- a/packages/next/src/server/app-render/create-error-handler.tsx +++ b/packages/next/src/server/app-render/create-error-handler.tsx @@ -114,17 +114,15 @@ export function createErrorHandler({ source === 'html') || source === 'flightData' ) { - // Dev mode logging: // The error logger is currently not provided in the edge runtime. // Use the exposed `__next_log_error__` instead. // This will trace error traces to the original source code. if (typeof __next_log_error__ === 'function') { __next_log_error__(err) - } else if (onReactStreamRenderError) { - onReactStreamRenderError(err) } else { console.error(err) } + onReactStreamRenderError?.(err) } } diff --git a/packages/next/src/server/next-server.ts b/packages/next/src/server/next-server.ts index 62eacd54a43c1..6c5a1354d104c 100644 --- a/packages/next/src/server/next-server.ts +++ b/packages/next/src/server/next-server.ts @@ -542,7 +542,7 @@ export default class NextNodeServer extends BaseServer< query, params: match.params, page: match.definition.pathname, - onError: this.instrumentationOnRequestError, + onError: this.instrumentationOnRequestError.bind(this), }) return true @@ -976,18 +976,27 @@ export default class NextNodeServer extends BaseServer< delete query._nextBubbleNoFallback delete query[NEXT_RSC_UNION_QUERY] - const handled = await this.runEdgeFunction({ - req, - res, - query, - params: match.params, - page: match.definition.page, - match, - appPaths: null, - }) - // If we handled the request, we can return early. - if (handled) return true + // For api routes edge runtime + try { + const handled = await this.runEdgeFunction({ + req, + res, + query, + params: match.params, + page: match.definition.page, + match, + appPaths: null, + }) + if (handled) return true + } catch (apiError) { + this.instrumentationOnRequestError(apiError, req, { + routePath: match.definition.page, + routerKind: 'Pages Router', + routeType: 'route', + }) + throw apiError + } } // If the route was detected as being a Pages API route, then handle From 8ddee8e288f2d1ee633829f9a6e57bcd595a5646 Mon Sep 17 00:00:00 2001 From: Jiachi Liu Date: Mon, 8 Jul 2024 14:00:54 +0200 Subject: [PATCH 17/27] fix duplicated logs --- packages/next/src/server/app-render/create-error-handler.tsx | 2 -- 1 file changed, 2 deletions(-) diff --git a/packages/next/src/server/app-render/create-error-handler.tsx b/packages/next/src/server/app-render/create-error-handler.tsx index c8ba43f8959d2..7cf52f7d77a14 100644 --- a/packages/next/src/server/app-render/create-error-handler.tsx +++ b/packages/next/src/server/app-render/create-error-handler.tsx @@ -119,8 +119,6 @@ export function createErrorHandler({ // This will trace error traces to the original source code. if (typeof __next_log_error__ === 'function') { __next_log_error__(err) - } else { - console.error(err) } onReactStreamRenderError?.(err) } From ceb417c81c2424117e0b75c71aebca1daeb36512 Mon Sep 17 00:00:00 2001 From: Jiachi Liu Date: Mon, 8 Jul 2024 14:48:44 +0200 Subject: [PATCH 18/27] fix prod log --- packages/next/src/server/app-render/create-error-handler.tsx | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/next/src/server/app-render/create-error-handler.tsx b/packages/next/src/server/app-render/create-error-handler.tsx index 7cf52f7d77a14..416cad1c0606c 100644 --- a/packages/next/src/server/app-render/create-error-handler.tsx +++ b/packages/next/src/server/app-render/create-error-handler.tsx @@ -117,8 +117,11 @@ export function createErrorHandler({ // The error logger is currently not provided in the edge runtime. // Use the exposed `__next_log_error__` instead. // This will trace error traces to the original source code. + if (typeof __next_log_error__ === 'function') { __next_log_error__(err) + } else if (!dev) { + console.error(err) } onReactStreamRenderError?.(err) } From 07ba7c82e164dbe2d1ffc0aedf8bf1f8126f7ec1 Mon Sep 17 00:00:00 2001 From: Jiachi Liu Date: Mon, 8 Jul 2024 19:47:44 +0200 Subject: [PATCH 19/27] fix log and middleware error stack --- packages/next/src/build/templates/middleware.ts | 2 +- packages/next/src/server/app-render/create-error-handler.tsx | 2 +- packages/next/types/$$compiled.internal.d.ts | 1 - packages/next/types/compiled.d.ts | 1 - test/development/middleware-errors/index.test.ts | 4 ++-- 5 files changed, 4 insertions(+), 6 deletions(-) diff --git a/packages/next/src/build/templates/middleware.ts b/packages/next/src/build/templates/middleware.ts index 7dfffc780ec10..440c293ceeaca 100644 --- a/packages/next/src/build/templates/middleware.ts +++ b/packages/next/src/build/templates/middleware.ts @@ -22,7 +22,7 @@ if (typeof handler !== 'function') { // Middleware will only sent out the FetchEvent to next server, // so load instrumentation module here and track the error inside middleware module. function errorHandledHandler(fn: AdapterOptions['handler']) { - return async function (...args: Parameters) { + return async (...args: Parameters) => { try { return await fn(...args) } catch (error) { diff --git a/packages/next/src/server/app-render/create-error-handler.tsx b/packages/next/src/server/app-render/create-error-handler.tsx index 416cad1c0606c..74054d7e234e5 100644 --- a/packages/next/src/server/app-render/create-error-handler.tsx +++ b/packages/next/src/server/app-render/create-error-handler.tsx @@ -120,7 +120,7 @@ export function createErrorHandler({ if (typeof __next_log_error__ === 'function') { __next_log_error__(err) - } else if (!dev) { + } else { console.error(err) } onReactStreamRenderError?.(err) diff --git a/packages/next/types/$$compiled.internal.d.ts b/packages/next/types/$$compiled.internal.d.ts index d2d701afa30f0..18d4b9f961815 100644 --- a/packages/next/types/$$compiled.internal.d.ts +++ b/packages/next/types/$$compiled.internal.d.ts @@ -32,7 +32,6 @@ declare module 'react-server-dom-webpack/server.node' declare module 'react-server-dom-webpack/client.edge' declare module 'VAR_MODULE_GLOBAL_ERROR' -declare module 'VAR_MODULE_INSTRUMENTATION' declare module 'VAR_USERLAND' declare module 'VAR_MODULE_DOCUMENT' declare module 'VAR_MODULE_APP' diff --git a/packages/next/types/compiled.d.ts b/packages/next/types/compiled.d.ts index 305507b655f4b..7d6707c1e6571 100644 --- a/packages/next/types/compiled.d.ts +++ b/packages/next/types/compiled.d.ts @@ -53,4 +53,3 @@ declare module 'VAR_MODULE_GLOBAL_ERROR' declare module 'VAR_USERLAND' declare module 'VAR_MODULE_DOCUMENT' declare module 'VAR_MODULE_APP' -declare module 'VAR_MODULE_INSTRUMENTATION' diff --git a/test/development/middleware-errors/index.test.ts b/test/development/middleware-errors/index.test.ts index fa74e35223973..16960d5c0f4e5 100644 --- a/test/development/middleware-errors/index.test.ts +++ b/test/development/middleware-errors/index.test.ts @@ -35,11 +35,11 @@ describe('middleware - development errors', () => { await check(() => { if (process.env.TURBOPACK) { expect(stripAnsi(next.cliOutput)).toMatch( - /middleware.js \(\d+:\d+\) @ Object.__TURBOPACK__default__export__ \[as handler\]/ + /middleware.js \(\d+:\d+\) @ __TURBOPACK__default__export__/ ) } else { expect(stripAnsi(next.cliOutput)).toMatch( - /middleware.js \(\d+:\d+\) @ Object.default \[as handler\]/ + /middleware.js \(\d+:\d+\) @ default/ ) } From 8ade0ce638a076b305a01e53f6287bd7ea5ae45f Mon Sep 17 00:00:00 2001 From: Jiachi Liu Date: Mon, 8 Jul 2024 20:10:32 +0200 Subject: [PATCH 20/27] fix dev logs --- .../next/src/server/app-render/create-error-handler.tsx | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/next/src/server/app-render/create-error-handler.tsx b/packages/next/src/server/app-render/create-error-handler.tsx index 74054d7e234e5..00e1857dd8389 100644 --- a/packages/next/src/server/app-render/create-error-handler.tsx +++ b/packages/next/src/server/app-render/create-error-handler.tsx @@ -118,8 +118,10 @@ export function createErrorHandler({ // Use the exposed `__next_log_error__` instead. // This will trace error traces to the original source code. - if (typeof __next_log_error__ === 'function') { - __next_log_error__(err) + if (process.env.NODE_ENV !== 'production') { + if (typeof __next_log_error__ === 'function') { + __next_log_error__(err) + } } else { console.error(err) } From 06f08fcb80adca16e3a6c917214c507b93c512f9 Mon Sep 17 00:00:00 2001 From: Jiachi Liu Date: Mon, 8 Jul 2024 20:52:10 +0200 Subject: [PATCH 21/27] fix dev logs --- .../next/src/server/app-render/create-error-handler.tsx | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/packages/next/src/server/app-render/create-error-handler.tsx b/packages/next/src/server/app-render/create-error-handler.tsx index 00e1857dd8389..e21949b7694a1 100644 --- a/packages/next/src/server/app-render/create-error-handler.tsx +++ b/packages/next/src/server/app-render/create-error-handler.tsx @@ -118,11 +118,9 @@ export function createErrorHandler({ // Use the exposed `__next_log_error__` instead. // This will trace error traces to the original source code. - if (process.env.NODE_ENV !== 'production') { - if (typeof __next_log_error__ === 'function') { - __next_log_error__(err) - } - } else { + if (typeof __next_log_error__ === 'function') { + __next_log_error__(err) + } else if (!dev && process.env.NEXT_RUNTIME !== 'edge') { console.error(err) } onReactStreamRenderError?.(err) From 033b317dc918ff87e10d253b7ee48072a65e8da3 Mon Sep 17 00:00:00 2001 From: Jiachi Liu Date: Mon, 8 Jul 2024 21:40:18 +0200 Subject: [PATCH 22/27] separate customized logs for different server --- .../next/src/server/app-render/app-render.tsx | 6 +++--- .../server/app-render/create-error-handler.tsx | 5 ----- .../next/src/server/dev/next-dev-server.ts | 18 +++++++++++------- packages/next/src/server/next-server.ts | 12 ++++++++++++ packages/next/src/server/web-server.ts | 17 +++++++++++++++++ 5 files changed, 43 insertions(+), 15 deletions(-) diff --git a/packages/next/src/server/app-render/app-render.tsx b/packages/next/src/server/app-render/app-render.tsx index bc32845567caa..b4f1e3d413900 100644 --- a/packages/next/src/server/app-render/app-render.tsx +++ b/packages/next/src/server/app-render/app-render.tsx @@ -693,9 +693,9 @@ async function renderToHTMLOrFlightImpl( routeType: 'render', } as const - function onReactStreamRenderError(err: Error) { - onInstrumentationRequestError?.(err, req, errorContext) - } + const onReactStreamRenderError = onInstrumentationRequestError + ? (err: Error) => onInstrumentationRequestError(err, req, errorContext) + : undefined const serverComponentsErrorHandler = createErrorHandler({ source: ErrorHandlerSource.serverComponents, diff --git a/packages/next/src/server/app-render/create-error-handler.tsx b/packages/next/src/server/app-render/create-error-handler.tsx index e21949b7694a1..6578ea4525947 100644 --- a/packages/next/src/server/app-render/create-error-handler.tsx +++ b/packages/next/src/server/app-render/create-error-handler.tsx @@ -118,11 +118,6 @@ export function createErrorHandler({ // Use the exposed `__next_log_error__` instead. // This will trace error traces to the original source code. - if (typeof __next_log_error__ === 'function') { - __next_log_error__(err) - } else if (!dev && process.env.NEXT_RUNTIME !== 'edge') { - console.error(err) - } onReactStreamRenderError?.(err) } } diff --git a/packages/next/src/server/dev/next-dev-server.ts b/packages/next/src/server/dev/next-dev-server.ts index 315ac3f9d6c61..9a1064c954423 100644 --- a/packages/next/src/server/dev/next-dev-server.ts +++ b/packages/next/src/server/dev/next-dev-server.ts @@ -65,6 +65,7 @@ import { isPostpone } from '../lib/router-utils/is-postpone' import { generateInterceptionRoutesRewrites } from '../../lib/generate-interception-routes-rewrites' import { buildCustomRoute } from '../../lib/build-custom-route' import { decorateServerError } from '../../shared/lib/error-source' +import type { ServerOnInstrumentationRequestError } from '../app-render/types' // Load ReactDevOverlay only when needed let ReactDevOverlayImpl: FunctionComponent @@ -186,13 +187,6 @@ export default class DevServer extends Server { }) } - this.renderOpts.onInstrumentationRequestError = (...args) => { - const [err, req, context] = args - super.instrumentationOnRequestError(err, req, context) - // Safe catch to avoid floating promises - this.logErrorWithOriginalStack(err, 'app-dir').catch(() => {}) - } - const { pagesDir, appDir } = findPagesDir(this.dir) this.pagesDir = pagesDir this.appDir = appDir @@ -867,4 +861,14 @@ export default class DevServer extends Server { async getCompilationError(page: string): Promise { return await this.bundlerService.getCompilationError(page) } + + protected async instrumentationOnRequestError( + ...args: Parameters + ) { + super.instrumentationOnRequestError(...args) + + const err = args[0] + // Safe catch to avoid floating promises + this.logErrorWithOriginalStack(err, 'app-dir').catch(() => {}) + } } diff --git a/packages/next/src/server/next-server.ts b/packages/next/src/server/next-server.ts index 6c5a1354d104c..2004ef893f8d2 100644 --- a/packages/next/src/server/next-server.ts +++ b/packages/next/src/server/next-server.ts @@ -105,6 +105,7 @@ import { formatDynamicImportPath } from '../lib/format-dynamic-import-path' import type { NextFontManifest } from '../build/webpack/plugins/next-font-manifest-plugin' import { isInterceptionRouteRewrite } from '../lib/generate-interception-routes-rewrites' import { stripNextRscUnionQuery } from '../lib/url' +import type { ServerOnInstrumentationRequestError } from './app-render/types' export * from './base-server' @@ -1973,4 +1974,15 @@ export default class NextNodeServer extends BaseServer< // development server. return null } + + protected async instrumentationOnRequestError( + ...args: Parameters + ) { + super.instrumentationOnRequestError(...args) + + // For Node.js runtime production logs, in dev it will be overridden by next-dev-server + if (!this.renderOpts.dev) { + console.error(args[0]) + } + } } diff --git a/packages/next/src/server/web-server.ts b/packages/next/src/server/web-server.ts index 0ececbad31cf1..bcbe3c20b006b 100644 --- a/packages/next/src/server/web-server.ts +++ b/packages/next/src/server/web-server.ts @@ -35,6 +35,7 @@ import { buildCustomRoute } from '../lib/build-custom-route' import { UNDERSCORE_NOT_FOUND_ROUTE } from '../api/constants' import type { DeepReadonly } from '../shared/lib/deep-readonly' import { getEdgeInstrumentationModule } from './web/globals' +import type { ServerOnInstrumentationRequestError } from './app-render/types' interface WebServerOptions extends Options { webServerConfig: { @@ -418,4 +419,20 @@ export default class NextWebServer extends BaseServer< protected async loadInstrumentationModule() { return await getEdgeInstrumentationModule() } + + protected async instrumentationOnRequestError( + ...args: Parameters + ) { + super.instrumentationOnRequestError(...args) + const err = args[0] + + if ( + process.env.NODE_ENV !== 'production' && + typeof __next_log_error__ === 'function' + ) { + __next_log_error__(err) + } else { + console.error(err) + } + } } From ea8808c1f39bb94c0029f3c57cbd3307a4fee4f0 Mon Sep 17 00:00:00 2001 From: Jiachi Liu Date: Mon, 8 Jul 2024 21:51:17 +0200 Subject: [PATCH 23/27] handle quiet option --- packages/next/src/server/next-server.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/next/src/server/next-server.ts b/packages/next/src/server/next-server.ts index 2004ef893f8d2..f689856031ac6 100644 --- a/packages/next/src/server/next-server.ts +++ b/packages/next/src/server/next-server.ts @@ -1982,7 +1982,7 @@ export default class NextNodeServer extends BaseServer< // For Node.js runtime production logs, in dev it will be overridden by next-dev-server if (!this.renderOpts.dev) { - console.error(args[0]) + this.logError(args[0] as Error) } } } From 8dcaf649ec4a5e91f5063c75e0985d7b91b9e45f Mon Sep 17 00:00:00 2001 From: Jiachi Liu Date: Tue, 9 Jul 2024 23:26:37 +0200 Subject: [PATCH 24/27] scope under an experimental env var --- .../next/src/build/templates/middleware.ts | 5 ++-- packages/next/src/server/base-server.ts | 29 ++++++++++--------- packages/next/src/server/web/globals.ts | 17 ++++++++++- test/e2e/on-request-error/basic/basic.test.ts | 3 ++ 4 files changed, 37 insertions(+), 17 deletions(-) diff --git a/packages/next/src/build/templates/middleware.ts b/packages/next/src/build/templates/middleware.ts index 440c293ceeaca..34fbb32b4c31c 100644 --- a/packages/next/src/build/templates/middleware.ts +++ b/packages/next/src/build/templates/middleware.ts @@ -6,7 +6,7 @@ import { adapter } from '../../server/web/adapter' // Import the userland code. import * as _mod from 'VAR_USERLAND' -import { getEdgeInstrumentationModule } from '../../server/web/globals' +import { edgeInstrumentationOnRequestError } from '../../server/web/globals' const mod = { ..._mod } const handler = mod.middleware || mod.default @@ -27,8 +27,7 @@ function errorHandledHandler(fn: AdapterOptions['handler']) { return await fn(...args) } catch (error) { const req = args[0] - const instrumentation = await getEdgeInstrumentationModule() - instrumentation?.onRequestError?.( + edgeInstrumentationOnRequestError( error, { url: req.url, diff --git a/packages/next/src/server/base-server.ts b/packages/next/src/server/base-server.ts index 4dc0904f56401..3a024b35a16e4 100644 --- a/packages/next/src/server/base-server.ts +++ b/packages/next/src/server/base-server.ts @@ -171,7 +171,7 @@ export interface MiddlewareRoutingItem { export type RouteHandler< ServerRequest extends BaseNextRequest = BaseNextRequest, - ServerResponse extends BaseNextResponse = BaseNextResponse, + ServerResponse extends BaseNextResponse = BaseNextResponse > = ( req: ServerRequest, res: ServerResponse, @@ -266,7 +266,7 @@ export interface BaseRequestHandler< ServerRequest extends BaseNextRequest | IncomingMessage = BaseNextRequest, ServerResponse extends | BaseNextResponse - | HTTPServerResponse = BaseNextResponse, + | HTTPServerResponse = BaseNextResponse > { ( req: ServerRequest, @@ -277,7 +277,7 @@ export interface BaseRequestHandler< export type RequestContext< ServerRequest extends BaseNextRequest = BaseNextRequest, - ServerResponse extends BaseNextResponse = BaseNextResponse, + ServerResponse extends BaseNextResponse = BaseNextResponse > = { req: ServerRequest res: ServerResponse @@ -315,7 +315,7 @@ export type NextEnabledDirectories = { export default abstract class Server< ServerOptions extends Options = Options, ServerRequest extends BaseNextRequest = BaseNextRequest, - ServerResponse extends BaseNextResponse = BaseNextResponse, + ServerResponse extends BaseNextResponse = BaseNextResponse > { public readonly hostname?: string public readonly fetchHostname?: string @@ -829,7 +829,10 @@ export default abstract class Server< ) { const [err, req, ctx] = args - if (this.instrumentation) { + if ( + process.env.__NEXT_EXPERIMENTAL_INSTRUMENTATION && + this.instrumentation + ) { this.instrumentation.onRequestError?.( err, { @@ -981,8 +984,8 @@ export default abstract class Server< req.headers['x-forwarded-port'] ??= this.port ? this.port.toString() : isHttps - ? '443' - : '80' + ? '443' + : '80' req.headers['x-forwarded-proto'] ??= isHttps ? 'https' : 'http' req.headers['x-forwarded-for'] ??= originalRequest?.socket?.remoteAddress @@ -1816,8 +1819,8 @@ export default abstract class Server< typeof fallbackField === 'string' ? 'static' : fallbackField === null - ? 'blocking' - : fallbackField, + ? 'blocking' + : fallbackField, } } @@ -2884,10 +2887,10 @@ export default abstract class Server< isOnDemandRevalidate ? 'REVALIDATED' : cacheEntry.isMiss - ? 'MISS' - : cacheEntry.isStale - ? 'STALE' - : 'HIT' + ? 'MISS' + : cacheEntry.isStale + ? 'STALE' + : 'HIT' ) } diff --git a/packages/next/src/server/web/globals.ts b/packages/next/src/server/web/globals.ts index 500f5cdf6ab26..91b618ac3de65 100644 --- a/packages/next/src/server/web/globals.ts +++ b/packages/next/src/server/web/globals.ts @@ -1,4 +1,7 @@ -import type { InstrumentationModule } from '../instrumentation/types' +import type { + InstrumentationModule, + InstrumentationOnRequestError, +} from '../instrumentation/types' declare const _ENTRIES: any @@ -29,6 +32,18 @@ async function registerInstrumentation() { } } +export async function edgeInstrumentationOnRequestError( + ...args: Parameters +) { + const instrumentation = await getEdgeInstrumentationModule() + if ( + process.env.__NEXT_EXPERIMENTAL_INSTRUMENTATION && + instrumentation?.onRequestError + ) { + instrumentation.onRequestError(...args) + } +} + let registerInstrumentationPromise: Promise | null = null export function ensureInstrumentationRegistered() { if (!registerInstrumentationPromise) { diff --git a/test/e2e/on-request-error/basic/basic.test.ts b/test/e2e/on-request-error/basic/basic.test.ts index 84dc65065387d..426b62db5bd77 100644 --- a/test/e2e/on-request-error/basic/basic.test.ts +++ b/test/e2e/on-request-error/basic/basic.test.ts @@ -4,6 +4,9 @@ import { retry } from 'next-test-utils' describe('on-request-error - basic', () => { const { next } = nextTestSetup({ files: __dirname, + env: { + __NEXT_EXPERIMENTAL_INSTRUMENTATION: '1', + }, }) const outputLogPath = 'output-log.json' From f87f9c4e48533baa74cd105007d19aa7aaba163a Mon Sep 17 00:00:00 2001 From: Jiachi Liu Date: Tue, 9 Jul 2024 23:32:06 +0200 Subject: [PATCH 25/27] lint format --- packages/next/src/server/base-server.ts | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/packages/next/src/server/base-server.ts b/packages/next/src/server/base-server.ts index 3a024b35a16e4..b9bb999efd523 100644 --- a/packages/next/src/server/base-server.ts +++ b/packages/next/src/server/base-server.ts @@ -171,7 +171,7 @@ export interface MiddlewareRoutingItem { export type RouteHandler< ServerRequest extends BaseNextRequest = BaseNextRequest, - ServerResponse extends BaseNextResponse = BaseNextResponse + ServerResponse extends BaseNextResponse = BaseNextResponse, > = ( req: ServerRequest, res: ServerResponse, @@ -266,7 +266,7 @@ export interface BaseRequestHandler< ServerRequest extends BaseNextRequest | IncomingMessage = BaseNextRequest, ServerResponse extends | BaseNextResponse - | HTTPServerResponse = BaseNextResponse + | HTTPServerResponse = BaseNextResponse, > { ( req: ServerRequest, @@ -277,7 +277,7 @@ export interface BaseRequestHandler< export type RequestContext< ServerRequest extends BaseNextRequest = BaseNextRequest, - ServerResponse extends BaseNextResponse = BaseNextResponse + ServerResponse extends BaseNextResponse = BaseNextResponse, > = { req: ServerRequest res: ServerResponse @@ -315,7 +315,7 @@ export type NextEnabledDirectories = { export default abstract class Server< ServerOptions extends Options = Options, ServerRequest extends BaseNextRequest = BaseNextRequest, - ServerResponse extends BaseNextResponse = BaseNextResponse + ServerResponse extends BaseNextResponse = BaseNextResponse, > { public readonly hostname?: string public readonly fetchHostname?: string @@ -984,8 +984,8 @@ export default abstract class Server< req.headers['x-forwarded-port'] ??= this.port ? this.port.toString() : isHttps - ? '443' - : '80' + ? '443' + : '80' req.headers['x-forwarded-proto'] ??= isHttps ? 'https' : 'http' req.headers['x-forwarded-for'] ??= originalRequest?.socket?.remoteAddress @@ -1819,8 +1819,8 @@ export default abstract class Server< typeof fallbackField === 'string' ? 'static' : fallbackField === null - ? 'blocking' - : fallbackField, + ? 'blocking' + : fallbackField, } } @@ -2887,10 +2887,10 @@ export default abstract class Server< isOnDemandRevalidate ? 'REVALIDATED' : cacheEntry.isMiss - ? 'MISS' - : cacheEntry.isStale - ? 'STALE' - : 'HIT' + ? 'MISS' + : cacheEntry.isStale + ? 'STALE' + : 'HIT' ) } From 5ec08d8b8a738aa65dcdeaf8bed91f14675c6ad9 Mon Sep 17 00:00:00 2001 From: Jiachi Liu Date: Wed, 10 Jul 2024 12:47:14 +0200 Subject: [PATCH 26/27] ensure error handler is finished --- .../next/src/build/templates/middleware.ts | 9 ++--- packages/next/src/server/base-server.ts | 35 +++++++++++-------- .../next/src/server/dev/next-dev-server.ts | 2 +- packages/next/src/server/next-server.ts | 4 +-- packages/next/src/server/web-server.ts | 2 +- packages/next/src/server/web/globals.ts | 16 ++++++--- 6 files changed, 40 insertions(+), 28 deletions(-) diff --git a/packages/next/src/build/templates/middleware.ts b/packages/next/src/build/templates/middleware.ts index 34fbb32b4c31c..b86e976970f2f 100644 --- a/packages/next/src/build/templates/middleware.ts +++ b/packages/next/src/build/templates/middleware.ts @@ -25,10 +25,10 @@ function errorHandledHandler(fn: AdapterOptions['handler']) { return async (...args: Parameters) => { try { return await fn(...args) - } catch (error) { + } catch (err) { const req = args[0] - edgeInstrumentationOnRequestError( - error, + await edgeInstrumentationOnRequestError( + err, { url: req.url, method: req.method, @@ -40,7 +40,8 @@ function errorHandledHandler(fn: AdapterOptions['handler']) { routeType: 'middleware', } ) - throw error + + throw err } } } diff --git a/packages/next/src/server/base-server.ts b/packages/next/src/server/base-server.ts index b9bb999efd523..37833768d654a 100644 --- a/packages/next/src/server/base-server.ts +++ b/packages/next/src/server/base-server.ts @@ -833,19 +833,24 @@ export default abstract class Server< process.env.__NEXT_EXPERIMENTAL_INSTRUMENTATION && this.instrumentation ) { - this.instrumentation.onRequestError?.( - err, - { - url: req.url || '', - method: req.method || 'GET', - // Normalize middleware headers and other server request headers - headers: - req instanceof NextRequestHint - ? Object.fromEntries(req.headers.entries()) - : req.headers, - }, - ctx - ) + try { + await this.instrumentation.onRequestError?.( + err, + { + url: req.url || '', + method: req.method || 'GET', + // Normalize middleware headers and other server request headers + headers: + req instanceof NextRequestHint + ? Object.fromEntries(req.headers.entries()) + : req.headers, + }, + ctx + ) + } catch (handlerErr) { + // Log the soft error and continue, since errors can thrown from react stream handler + console.error('Error in instrumentation.onRequestError:', handlerErr) + } } } @@ -2495,7 +2500,7 @@ export default abstract class Server< Log.error(err) - this.instrumentationOnRequestError(err, req, { + await this.instrumentationOnRequestError(err, req, { routerKind: 'App Router', routePath: pathname, routeType: 'route', @@ -2545,7 +2550,7 @@ export default abstract class Server< } ) } catch (err) { - this.instrumentationOnRequestError(err, req, { + await this.instrumentationOnRequestError(err, req, { routerKind: 'Pages Router', routePath: pathname, routeType: 'render', diff --git a/packages/next/src/server/dev/next-dev-server.ts b/packages/next/src/server/dev/next-dev-server.ts index 9a1064c954423..8bf6d1bda01d0 100644 --- a/packages/next/src/server/dev/next-dev-server.ts +++ b/packages/next/src/server/dev/next-dev-server.ts @@ -865,7 +865,7 @@ export default class DevServer extends Server { protected async instrumentationOnRequestError( ...args: Parameters ) { - super.instrumentationOnRequestError(...args) + await super.instrumentationOnRequestError(...args) const err = args[0] // Safe catch to avoid floating promises diff --git a/packages/next/src/server/next-server.ts b/packages/next/src/server/next-server.ts index f689856031ac6..63a2c9407757a 100644 --- a/packages/next/src/server/next-server.ts +++ b/packages/next/src/server/next-server.ts @@ -991,7 +991,7 @@ export default class NextNodeServer extends BaseServer< }) if (handled) return true } catch (apiError) { - this.instrumentationOnRequestError(apiError, req, { + await this.instrumentationOnRequestError(apiError, req, { routePath: match.definition.page, routerKind: 'Pages Router', routeType: 'route', @@ -1978,7 +1978,7 @@ export default class NextNodeServer extends BaseServer< protected async instrumentationOnRequestError( ...args: Parameters ) { - super.instrumentationOnRequestError(...args) + await super.instrumentationOnRequestError(...args) // For Node.js runtime production logs, in dev it will be overridden by next-dev-server if (!this.renderOpts.dev) { diff --git a/packages/next/src/server/web-server.ts b/packages/next/src/server/web-server.ts index bcbe3c20b006b..7434c66f4902a 100644 --- a/packages/next/src/server/web-server.ts +++ b/packages/next/src/server/web-server.ts @@ -423,7 +423,7 @@ export default class NextWebServer extends BaseServer< protected async instrumentationOnRequestError( ...args: Parameters ) { - super.instrumentationOnRequestError(...args) + await super.instrumentationOnRequestError(...args) const err = args[0] if ( diff --git a/packages/next/src/server/web/globals.ts b/packages/next/src/server/web/globals.ts index 91b618ac3de65..637b6fafb5557 100644 --- a/packages/next/src/server/web/globals.ts +++ b/packages/next/src/server/web/globals.ts @@ -36,11 +36,17 @@ export async function edgeInstrumentationOnRequestError( ...args: Parameters ) { const instrumentation = await getEdgeInstrumentationModule() - if ( - process.env.__NEXT_EXPERIMENTAL_INSTRUMENTATION && - instrumentation?.onRequestError - ) { - instrumentation.onRequestError(...args) + try { + if ( + process.env.__NEXT_EXPERIMENTAL_INSTRUMENTATION && + instrumentation?.onRequestError + ) { + await instrumentation.onRequestError(...args) + } + } catch (err: any) { + // Throw hard error since middleware is critical path here, if it's error then the request will fail anyway. + err.message = `An error occurred while running instrumentation request error handler: ${err.message}` + throw err } } From 4fbf19b05c1940e210d2d46a964c60057acae1ab Mon Sep 17 00:00:00 2001 From: Jiachi Liu Date: Wed, 10 Jul 2024 12:48:52 +0200 Subject: [PATCH 27/27] change to console.error --- packages/next/src/server/web/globals.ts | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/packages/next/src/server/web/globals.ts b/packages/next/src/server/web/globals.ts index 637b6fafb5557..5e5ce7c8e56cc 100644 --- a/packages/next/src/server/web/globals.ts +++ b/packages/next/src/server/web/globals.ts @@ -43,10 +43,9 @@ export async function edgeInstrumentationOnRequestError( ) { await instrumentation.onRequestError(...args) } - } catch (err: any) { - // Throw hard error since middleware is critical path here, if it's error then the request will fail anyway. - err.message = `An error occurred while running instrumentation request error handler: ${err.message}` - throw err + } catch (err) { + // Log the soft error and continue, since the original error has already been thrown + console.error('Error in instrumentation.onRequestError:', err) } }