From 05010a4799c46d96ebd2eabac966eefb4ccbdb49 Mon Sep 17 00:00:00 2001 From: Felipeness Date: Thu, 20 Aug 2026 00:46:07 -0300 Subject: [PATCH] lib: window long source lines when building error messages getFirstExpression() tokenizes the whole source line to find the expression at the error column. In bundled or minified code that line is the entire file, so every failing assertion pays an O(line) tokenize, and the result is not cached. Extract a window around the target column for lines longer than 2048 characters, cutting at the previous statement boundary so acorn still receives usable input, and tolerate a window that starts mid-token. On a single 930KB line, 20 failing assertions go from 1118ms to 34ms. Refs: https://github.com/nodejs/node/issues/52677 Refs: https://github.com/nodejs/node/issues/52962 --- lib/internal/errors/error_source.js | 140 +++++++++++++------- test/parallel/test-assert-long-line-perf.js | 63 +++++++++ 2 files changed, 154 insertions(+), 49 deletions(-) create mode 100644 test/parallel/test-assert-long-line-perf.js diff --git a/lib/internal/errors/error_source.js b/lib/internal/errors/error_source.js index eddd6af23080..acd27bdb4eb1 100644 --- a/lib/internal/errors/error_source.js +++ b/lib/internal/errors/error_source.js @@ -2,6 +2,8 @@ const { FunctionPrototypeBind, + MathMax, + StringPrototypeLastIndexOf, StringPrototypeSlice, } = primordials; @@ -61,6 +63,18 @@ function getErrorSourceLocation(error) { const memberAccessTokens = [ '.', '?.', '[', ']' ]; const memberNameTokens = [ 'name', 'string', 'num' ]; let tokenizer; + +// Maximum source line length to tokenize. Lines longer than this are windowed +// around the target column to avoid O(n) tokenization of huge single-line +// minified/bundled files (see https://github.com/nodejs/node/issues/52677). +const kMaxSourceLineLength = 2048; +// Size of the lookback window before startColumn to capture member access +// expressions like `assert.ok` or `module.exports.assert['ok']`. +const kWindowLookback = 512; +// Size of the lookahead window after startColumn to capture the full +// expression including arguments, e.g. `assert.ok(false)`. +const kWindowLookahead = 512; + /** * Get the first expression in a code string at the startColumn. * @param {string} code source code line @@ -74,68 +88,96 @@ function getFirstExpression(code, startColumn) { tokenizer = FunctionPrototypeBind(Parser.tokenizer, Parser); } + // For long lines (common in minified/bundled code), extract a small window + // around the target column instead of tokenizing the entire line. + // This prevents O(n) tokenization that can hang for minutes on large files. + if (code.length > kMaxSourceLineLength) { + // Find a safe cut point by looking for the last semicolon before + // our lookback window start. Semicolons are statement boundaries, + // so cutting there gives acorn valid-ish input. + const windowStart = MathMax(0, startColumn - kWindowLookback); + let cutPoint = windowStart; + if (windowStart > 0) { + const lastSemicolon = StringPrototypeLastIndexOf(code, ';', windowStart); + // Use the position right after the semicolon as the cut point. + cutPoint = lastSemicolon !== -1 ? lastSemicolon + 1 : windowStart; + } + const windowEnd = startColumn + kWindowLookahead; + code = StringPrototypeSlice(code, cutPoint, windowEnd); + startColumn -= cutPoint; + } + let lastToken; let firstMemberAccessNameToken; let terminatingCol; let parenLvl = 0; // Tokenize the line to locate the expression at the startColumn. // The source line may be an incomplete JavaScript source, so do not parse the source line. - for (const token of tokenizer(code, { ecmaVersion: 'latest' })) { - // Peek before the startColumn. - if (token.start < startColumn) { - // There is a semicolon. This is a statement before the startColumn, so reset the memo. - if (token.type.label === ';') { - firstMemberAccessNameToken = null; + // Wrap in try-catch because the windowed code may start mid-token (e.g., inside a string + // literal), causing acorn to throw SyntaxError. + try { + for (const token of tokenizer(code, { ecmaVersion: 'latest' })) { + // Peek before the startColumn. + if (token.start < startColumn) { + // There is a semicolon. This is a statement before the startColumn, so reset the memo. + if (token.type.label === ';') { + firstMemberAccessNameToken = null; + continue; + } + // Try to memo the member access expressions before the startColumn, so that the + // returned source code contains more info: + // assert.ok(value) + // ^ startColumn + // The member expression can also be like + // assert['ok'](value) or assert?.ok(value) + // ^ startColumn ^ startColumn + if (memberAccessTokens.includes(token.type.label) && lastToken?.type.label === 'name') { + // First member access name token must be a 'name'. + firstMemberAccessNameToken ??= lastToken; + } else if (!memberAccessTokens.includes(token.type.label) && + !memberNameTokens.includes(token.type.label)) { + // Reset the memo if it is not a simple member access. + // For example: assert[(() => 'ok')()](value) + // ^ startColumn + firstMemberAccessNameToken = null; + } + lastToken = token; continue; } - // Try to memo the member access expressions before the startColumn, so that the - // returned source code contains more info: - // assert.ok(value) - // ^ startColumn - // The member expression can also be like - // assert['ok'](value) or assert?.ok(value) - // ^ startColumn ^ startColumn - if (memberAccessTokens.includes(token.type.label) && lastToken?.type.label === 'name') { - // First member access name token must be a 'name'. - firstMemberAccessNameToken ??= lastToken; - } else if (!memberAccessTokens.includes(token.type.label) && - !memberNameTokens.includes(token.type.label)) { - // Reset the memo if it is not a simple member access. - // For example: assert[(() => 'ok')()](value) - // ^ startColumn - firstMemberAccessNameToken = null; + // Now after the startColumn, this must be an expression. + if (token.type.label === '(') { + parenLvl++; + continue; } - lastToken = token; - continue; - } - // Now after the startColumn, this must be an expression. - if (token.type.label === '(') { - parenLvl++; - continue; - } - if (token.type.label === ')') { - parenLvl--; - if (parenLvl === 0) { - // A matched closing parenthesis found after the startColumn, - // terminate here. Include the token. - // (assert.ok(false), assert.ok(true)) - // ^ startColumn - terminatingCol = token.start + 1; + if (token.type.label === ')') { + parenLvl--; + if (parenLvl === 0) { + // A matched closing parenthesis found after the startColumn, + // terminate here. Include the token. + // (assert.ok(false), assert.ok(true)) + // ^ startColumn + terminatingCol = token.start + 1; + break; + } + continue; + } + if (token.type.label === ';') { + // A semicolon found after the startColumn, terminate here. + // assert.ok(false); assert.ok(true)); + // ^ startColumn + terminatingCol = token; break; } - continue; - } - if (token.type.label === ';') { - // A semicolon found after the startColumn, terminate here. - // assert.ok(false); assert.ok(true)); + // If no semicolon found after the startColumn. The string after the + // startColumn must be the expression. + // assert.ok(false) // ^ startColumn - terminatingCol = token; - break; } - // If no semicolon found after the startColumn. The string after the - // startColumn must be the expression. - // assert.ok(false) - // ^ startColumn + } catch { + // The windowed code may be invalid JavaScript (e.g., starting mid-string-literal). + // Return undefined so the caller emits a message without source expression + // instead of returning garbage from partially-tokenized input. + return undefined; } const start = firstMemberAccessNameToken?.start ?? startColumn; return StringPrototypeSlice(code, start, terminatingCol); diff --git a/test/parallel/test-assert-long-line-perf.js b/test/parallel/test-assert-long-line-perf.js new file mode 100644 index 000000000000..82a3370135a4 --- /dev/null +++ b/test/parallel/test-assert-long-line-perf.js @@ -0,0 +1,63 @@ +'use strict'; + +// Verify that assert.ok with minified/bundled single-line code does not hang. +// Regression test for https://github.com/nodejs/node/issues/52677 + +require('../common'); +const tmpdir = require('../common/tmpdir'); +tmpdir.refresh(); + +const assert = require('assert'); +const { spawnSync } = require('child_process'); +const fs = require('fs'); +const path = require('path'); +const { test } = require('node:test'); + +test('assert.ok completes quickly on long single-line source', () => { + // Generate a single-line file that exceeds kMaxSourceLineLength (2048) + // with semicolons as statement boundaries, followed by a failing assert. + // ~3000 chars is enough to trigger windowing without stressing the filesystem. + const padding = 'var x=1;'.repeat(375); + const code = padding + "require('node:assert').ok(false)"; + + const file = path.join(tmpdir.path, 'assert-minified-perf.js'); + fs.writeFileSync(file, code); + + const start = process.hrtime.bigint(); + const result = spawnSync(process.execPath, [file], { + timeout: 30_000, + encoding: 'utf8', + }); + const elapsedMs = Number(process.hrtime.bigint() - start) / 1e6; + + // The process should exit with code 1 (assertion error), not be killed + // by the timeout signal. + assert.strictEqual(result.signal, null, + 'Process was killed by timeout - assert.ok hung on long line'); + assert.strictEqual(result.status, 1); + assert.match(result.stderr, /AssertionError/); + + // With windowing, this should complete in well under 10 seconds even on + // slow CI machines. Without windowing, long lines can take minutes. + assert.ok(elapsedMs < 10_000, + `Expected completion in <10s, took ${elapsedMs.toFixed(0)}ms`); +}); + +test('assert.ok error message is correct for long single-line source', () => { + // A long line with semicolons followed by a failing assert. The windowing + // logic should find the semicolons and extract the expression correctly. + const padding = ';'.repeat(3000); + const code = padding + "require('node:assert').ok(false)"; + + const file = path.join(tmpdir.path, 'assert-long-line-msg.js'); + fs.writeFileSync(file, code); + + const result = spawnSync(process.execPath, [file], { + timeout: 30_000, + encoding: 'utf8', + }); + + assert.strictEqual(result.signal, null); + assert.strictEqual(result.status, 1); + assert.match(result.stderr, /ok\(false\)/); +});