From 592e27831888ceefb9b6730b59366cc2ae8d69ce Mon Sep 17 00:00:00 2001 From: MrBBot Date: Wed, 30 Nov 2022 09:44:19 +0000 Subject: [PATCH] Log uncaught source-mapped errors to the console (#446) Previously, errors _thrown_ using the `MF-Experimental-Error-Stack` header were only shown in a pretty-error page. This PR logs those errors with source-maps applied to the console too, using the `source-map-support` package. This simplifies our code, and also means we don't need to touch internal Youch methods, as the errors we pass to Youch are already source-mapped. --- packages/miniflare/package.json | 3 +- packages/miniflare/src/index.ts | 6 +- .../src/plugins/core/errors/callsite.ts | 157 ++++++++++++++++++ .../core/{prettyerror.ts => errors/index.ts} | 153 ++++------------- .../src/plugins/core/errors/sourcemap.ts | 66 ++++++++ packages/miniflare/src/plugins/core/index.ts | 4 +- 6 files changed, 265 insertions(+), 124 deletions(-) create mode 100644 packages/miniflare/src/plugins/core/errors/callsite.ts rename packages/miniflare/src/plugins/core/{prettyerror.ts => errors/index.ts} (63%) create mode 100644 packages/miniflare/src/plugins/core/errors/sourcemap.ts diff --git a/packages/miniflare/package.json b/packages/miniflare/package.json index 789ca237a961b..5687e48598070 100644 --- a/packages/miniflare/package.json +++ b/packages/miniflare/package.json @@ -35,7 +35,7 @@ "glob-to-regexp": "^0.4.1", "http-cache-semantics": "^4.1.0", "kleur": "^4.1.5", - "source-map": "^0.7.4", + "source-map-support": "0.5.21", "stoppable": "^1.1.0", "undici": "^5.12.0", "workerd": "^1.20221111.5", @@ -49,6 +49,7 @@ "@types/estree": "^1.0.0", "@types/glob-to-regexp": "^0.4.1", "@types/http-cache-semantics": "^4.0.1", + "@types/source-map-support": "^0.5.6", "@types/stoppable": "^1.1.1", "@types/ws": "^8.5.3" }, diff --git a/packages/miniflare/src/index.ts b/packages/miniflare/src/index.ts index 87b93179fc3d2..dcebfb05256b2 100644 --- a/packages/miniflare/src/index.ts +++ b/packages/miniflare/src/index.ts @@ -360,7 +360,11 @@ export class Miniflare { const workerSrcOpts = this.#workerOpts.map( ({ core }) => core ); - response = await handlePrettyErrorRequest(workerSrcOpts, request); + response = await handlePrettyErrorRequest( + this.#log, + workerSrcOpts, + request + ); } else { // TODO: check for proxying/outbound fetch header first (with plans for fetch mocking) response = await this.#handleLoopbackPlugins(request, url); diff --git a/packages/miniflare/src/plugins/core/errors/callsite.ts b/packages/miniflare/src/plugins/core/errors/callsite.ts new file mode 100644 index 0000000000000..1a44c40fd0ac2 --- /dev/null +++ b/packages/miniflare/src/plugins/core/errors/callsite.ts @@ -0,0 +1,157 @@ +// Lifted from `node-stack-trace`: +// https://github.com/felixge/node-stack-trace/blob/4c41a4526e74470179b3b6dd5d75191ca8c56c17/index.js +// Ideally, we'd just use this package as-is, but it has a strict +// `engines.node == 16` constraint in its `package.json`. There's a PR open to +// fix this (https://github.com/felixge/node-stack-trace/pull/39), but it's been +// open for a while. As soon as it's merged, we should just depend on it. + +/*! + * Copyright (c) 2011 Felix Geisendörfer (felix@debuggable.com) + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + */ + +export function parseStack(stack: string): CallSite[] { + return stack + .split("\n") + .slice(1) + .map(parseCallSite) + .filter((site): site is CallSite => site !== undefined); +} + +function parseCallSite(line: string): CallSite | undefined { + const lineMatch = line.match( + /at (?:(.+?)\s+\()?(?:(.+?):(\d+)(?::(\d+))?|([^)]+))\)?/ + ); + if (!lineMatch) { + return; + } + + let object = null; + let method = null; + let functionName = null; + let typeName = null; + let methodName = null; + const isNative = lineMatch[5] === "native"; + + if (lineMatch[1]) { + functionName = lineMatch[1]; + let methodStart = functionName.lastIndexOf("."); + if (functionName[methodStart - 1] == ".") methodStart--; + if (methodStart > 0) { + object = functionName.substring(0, methodStart); + method = functionName.substring(methodStart + 1); + const objectEnd = object.indexOf(".Module"); + if (objectEnd > 0) { + functionName = functionName.substring(objectEnd + 1); + object = object.substring(0, objectEnd); + } + } + } + + if (method) { + typeName = object; + methodName = method; + } + + if (method === "") { + methodName = null; + functionName = null; + } + + return new CallSite({ + typeName, + functionName, + methodName, + fileName: lineMatch[2] || null, + lineNumber: parseInt(lineMatch[3]) || null, + columnNumber: parseInt(lineMatch[4]) || null, + native: isNative, + }); +} + +export interface CallSiteOptions { + typeName: string | null; + functionName: string | null; + methodName: string | null; + fileName: string | null; + lineNumber: number | null; + columnNumber: number | null; + native: boolean; +} + +// https://v8.dev/docs/stack-trace-api#customizing-stack-traces +// This class supports the subset of options implemented by `node-stack-trace`: +// https://github.com/felixge/node-stack-trace/blob/4c41a4526e74470179b3b6dd5d75191ca8c56c17/index.js +export class CallSite implements NodeJS.CallSite { + constructor(private readonly opts: CallSiteOptions) {} + + getThis(): unknown { + return null; + } + getTypeName(): string | null { + return this.opts.typeName; + } + // eslint-disable-next-line @typescript-eslint/ban-types + getFunction(): Function | undefined { + return undefined; + } + getFunctionName(): string | null { + return this.opts.functionName; + } + getMethodName(): string | null { + return this.opts.methodName; + } + getFileName(): string | null { + return this.opts.fileName; + } + getLineNumber(): number | null { + return this.opts.lineNumber; + } + getColumnNumber(): number | null { + return this.opts.columnNumber; + } + getEvalOrigin(): string | undefined { + return undefined; + } + isToplevel(): boolean { + return false; + } + isEval(): boolean { + return false; + } + isNative(): boolean { + return this.opts.native; + } + isConstructor(): boolean { + return false; + } + isAsync(): boolean { + return false; + } + isPromiseAll(): boolean { + return false; + } + isPromiseAny(): boolean { + return false; + } + getPromiseIndex(): number | null { + return null; + } +} diff --git a/packages/miniflare/src/plugins/core/prettyerror.ts b/packages/miniflare/src/plugins/core/errors/index.ts similarity index 63% rename from packages/miniflare/src/plugins/core/prettyerror.ts rename to packages/miniflare/src/plugins/core/errors/index.ts index 9864a9e56d6d8..601396e76a2fb 100644 --- a/packages/miniflare/src/plugins/core/prettyerror.ts +++ b/packages/miniflare/src/plugins/core/errors/index.ts @@ -1,15 +1,15 @@ import fs from "fs"; import path from "path"; -import { fileURLToPath } from "url"; -import { SourceMapConsumer } from "source-map"; -import type { Entry } from "stacktracey"; +import type { UrlAndMap } from "source-map-support"; import { Request, Response } from "undici"; import { z } from "zod"; +import { Log } from "../../../shared"; import { ModuleDefinition, contentsToString, maybeGetStringScriptPathIndex, -} from "./modules"; +} from "../modules"; +import { getSourceMapper } from "./sourcemap"; // Subset of core worker options that define Worker source code. // These are the possible cases, and corresponding reported source files in @@ -163,85 +163,30 @@ function maybeGetFile( return maybeGetDiskFile(filePath); } -// Try to find a source map for `sourceFile`, and update `sourceFile` and -// `frame` with source mapped locations -type SourceMapCache = Map>; -async function applySourceMaps( - /* mut */ sourceMapCache: SourceMapCache, - /* mut */ sourceFile: SourceFile, - /* mut */ frame: Entry -) { - // If we don't have a file path, or full location, we can't do any source - // mapping, so return straight away. - // TODO: support data URIs for maps, in `sourceFile`s without path - if ( - sourceFile.path === undefined || - frame.line === undefined || - frame.column === undefined - ) { - return; - } - - // Find the last source mapping URL if any - const sourceMapRegexp = /# sourceMappingURL=(.+)/g; - let sourceMapMatch: RegExpMatchArray | null = null; - while (true) { - const match = sourceMapRegexp.exec(sourceFile.contents); - if (match !== null) sourceMapMatch = match; - else break; - } - // If we couldn't find a source mapping URL, there's nothing we can do - if (sourceMapMatch === null) return; - - // Get the source map - const root = path.dirname(sourceFile.path); - const sourceMapPath = path.resolve(root, sourceMapMatch[1]); - let consumerPromise = sourceMapCache.get(sourceMapPath); - if (consumerPromise === undefined) { - // If we couldn't find the source map in cache, load it +function getSourceMappedStack(workerSrcOpts: SourceOptions[], error: Error) { + // This function needs to match the signature of the `retrieveSourceMap` + // option from the "source-map-support" package. + function retrieveSourceMap(file: string): UrlAndMap | null { + const sourceFile = maybeGetFile(workerSrcOpts, file); + if (sourceFile?.path === undefined) return null; + + // Find the last source mapping URL if any + const sourceMapRegexp = /# sourceMappingURL=(.+)/g; + const matches = [...sourceFile.contents.matchAll(sourceMapRegexp)]; + // If we couldn't find a source mapping URL, there's nothing we can do + if (matches.length === 0) return null; + const sourceMapMatch = matches[matches.length - 1]; + + // Get the source map + const root = path.dirname(sourceFile.path); + const sourceMapPath = path.resolve(root, sourceMapMatch[1]); const sourceMapFile = maybeGetDiskFile(sourceMapPath); - if (sourceMapFile === undefined) return; - const rawSourceMap = JSON.parse(sourceMapFile.contents); - consumerPromise = new SourceMapConsumer(rawSourceMap); - sourceMapCache.set(sourceMapPath, consumerPromise); - } - const consumer = await consumerPromise; + if (sourceMapFile === undefined) return null; - // Get original position from source map - const original = consumer.originalPositionFor({ - line: frame.line, - column: frame.column, - }); - // If source mapping failed, don't make changes - if ( - original.source === null || - original.line === null || - original.column === null - ) { - return; + return { map: sourceMapFile.contents }; } - // Update source file and frame with source mapped locations - const newSourceFile = maybeGetDiskFile(original.source); - if (newSourceFile === undefined) return; - sourceFile.path = original.source; - sourceFile.contents = newSourceFile.contents; - frame.file = original.source; - frame.fileRelative = path.relative("", sourceFile.path); - frame.fileShort = frame.fileRelative; - frame.fileName = path.basename(sourceFile.path); - frame.line = original.line; - frame.column = original.column; -} - -interface YouchInternalFrameSource { - pre: string[]; - line: string; - post: string[]; -} -interface YouchInternals { - options: { preLines: number; postLines: number }; - _getFrameSource(frame: Entry): Promise; + return getSourceMapper()(retrieveSourceMap, error); } // Due to a bug in `workerd`, if `Promise`s returned from native C++ APIs are @@ -257,6 +202,7 @@ const ErrorSchema = z.object({ stack: z.ostring(), }); export async function handlePrettyErrorRequest( + log: Log, workerSrcOpts: SourceOptions[], request: Request ): Promise { @@ -271,11 +217,11 @@ export async function handlePrettyErrorRequest( error.message = caught.message as string; error.stack = caught.stack; - // Create a source-map cache for this pretty-error request. We only cache per - // request, as it's likely the user will update their code and restart/call - // `setOptions` again on seeing this page. This would invalidate existing - // source maps. Keeping the cache per request simplifies things too. - const sourceMapCache: SourceMapCache = new Map(); + // Try to apply source-mapping + error.stack = getSourceMappedStack(workerSrcOpts, error); + + // Log source-mapped error to console if logging enabled + log.error(error); // Lazily import `youch` when required const Youch: typeof import("youch").default = require("youch"); @@ -290,40 +236,7 @@ export async function handlePrettyErrorRequest( '💬 Workers Discord', ].join(""); }); - const youchInternals = youch as unknown as YouchInternals; - youchInternals._getFrameSource = async (frame) => { - // Adapted from Youch's own implementation - let file = frame.file - .replace(/dist\/webpack:\//g, "") // Unix - .replace(/dist\\webpack:\\/g, ""); // Windows - // Ignore error as frame source is optional anyway - try { - file = file.startsWith("file:") ? fileURLToPath(file) : file; - } catch {} - - // Try get source-mapped file contents - const sourceFile = await maybeGetFile(workerSrcOpts, file); - if (sourceFile === undefined || frame.line === undefined) return null; - // If source-mapping fails, this function won't do anything - await applySourceMaps(sourceMapCache, sourceFile, frame); - - // Return lines around frame as required by Youch - const lines = sourceFile.contents.split(/\r?\n/); - const line = frame.line; - const pre = lines.slice( - Math.max(0, line - (youchInternals.options.preLines + 1)), - line - 1 - ); - const post = lines.slice(line, line + youchInternals.options.postLines); - return { pre, line: lines[line - 1], post }; - }; - - try { - return new Response(await youch.toHTML(), { - headers: { "Content-Type": "text/html;charset=utf-8" }, - }); - } finally { - // Clean-up source-map cache - for (const consumer of sourceMapCache.values()) (await consumer).destroy(); - } + return new Response(await youch.toHTML(), { + headers: { "Content-Type": "text/html;charset=utf-8" }, + }); } diff --git a/packages/miniflare/src/plugins/core/errors/sourcemap.ts b/packages/miniflare/src/plugins/core/errors/sourcemap.ts new file mode 100644 index 0000000000000..4f0bd5c989ea1 --- /dev/null +++ b/packages/miniflare/src/plugins/core/errors/sourcemap.ts @@ -0,0 +1,66 @@ +import assert from "assert"; +import type { Options } from "source-map-support"; +import { parseStack } from "./callsite"; + +const sourceMapInstallBaseOptions: Options = { + environment: "node", + // Don't add Node `uncaughtException` handler + handleUncaughtExceptions: false, + // Don't hook Node `require` function + hookRequire: false, + + // Make sure we're using fresh copies of files (i.e. between `setOptions()`) + emptyCacheBetweenOperations: true, + + // Always remove existing retrievers when calling `install()`, we should be + // specifying them each time we want to source map + overrideRetrieveFile: true, + overrideRetrieveSourceMap: true, +}; + +// Returns the source-mapped stack trace for the specified error, using the +// specified function for retrieving source-maps. +export type SourceMapper = ( + retrieveSourceMap: Options["retrieveSourceMap"], + error: Error +) => string; + +let sourceMapper: SourceMapper; +export function getSourceMapper(): SourceMapper { + if (sourceMapper !== undefined) return sourceMapper; + + // `source-map-support` will only modify `Error.prepareStackTrace` if this is + // the first time `install()` has been called. This is governed by a module + // level variable: `errorFormatterInstalled`. To ensure we're not affecting + // external user's use of this package, and so `Error.prepareStackTrace` is + // always updated, load a fresh copy, by resetting then restoring the + // `require` cache. + + const originalSupport = require.cache["source-map-support"]; + delete require.cache["source-map-support"]; + const support: typeof import("source-map-support") = require("source-map-support"); + require.cache["source-map-support"] = originalSupport; + + const originalPrepareStackTrace = Error.prepareStackTrace; + support.install(sourceMapInstallBaseOptions); + const prepareStackTrace = Error.prepareStackTrace; + assert(prepareStackTrace !== undefined); + Error.prepareStackTrace = originalPrepareStackTrace; + + sourceMapper = (retrieveSourceMap, error) => { + support.install({ + ...sourceMapInstallBaseOptions, + retrieveFile(file: string): string | null { + // `retrieveFile` should only be called by the default implementation of + // `retrieveSourceMap`, so we shouldn't need to implement it + assert.fail(`Unexpected retrieveFile(${JSON.stringify(file)}) call`); + }, + retrieveSourceMap, + }); + + // Parse the stack trace into structured call sites and source-map them + const callSites = parseStack(error.stack ?? ""); + return prepareStackTrace(error, callSites); + }; + return sourceMapper; +} diff --git a/packages/miniflare/src/plugins/core/index.ts b/packages/miniflare/src/plugins/core/index.ts index 88eda122bc2ac..d291833b2254d 100644 --- a/packages/miniflare/src/plugins/core/index.ts +++ b/packages/miniflare/src/plugins/core/index.ts @@ -17,6 +17,7 @@ import { CloudflareFetchSchema, Plugin, } from "../shared"; +import { HEADER_ERROR_STACK } from "./errors"; import { ModuleDefinitionSchema, ModuleLocator, @@ -24,7 +25,6 @@ import { buildStringScriptPath, convertModuleDefinition, } from "./modules"; -import { HEADER_ERROR_STACK } from "./prettyerror"; import { ServiceDesignatorSchema } from "./services"; const encoder = new TextEncoder(); @@ -473,5 +473,5 @@ function getWorkerScript( } } -export * from "./prettyerror"; +export * from "./errors"; export * from "./services";