From e421f650bf225bce56af85cc3c25b30709661bc6 Mon Sep 17 00:00:00 2001 From: Felipe Coelho Date: Thu, 19 Mar 2026 11:05:10 -0300 Subject: [PATCH 1/2] fix: slow error message generation for minified code When assert.ok(falsy) is called in minified/bundled code (common in Lambda deployments via Webpack/Terser), the error message generation can take seconds or even minutes. The getFirstExpression function tokenizes the entire source line with acorn to extract the failing expression, but for minified files the source line is the entire file content (potentially megabytes of code on a single line). Fix by windowing the tokenization: when the source line exceeds 2048 characters, extract a ~1024 character window around the target column instead of tokenizing the entire line. The window uses semicolons as safe cut points to give acorn valid-ish input. A try-catch is added to gracefully handle cases where the windowed code starts mid-token. This preserves the detailed error message (including member access like assert.ok) while bounding tokenization work to a constant size regardless of file length. Fixes: https://github.com/nodejs/node/issues/52677 --- lib/internal/errors/error_source.js | 138 +++++++++++++------- test/parallel/test-assert-long-line-perf.js | 61 +++++++++ 2 files changed, 150 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 eddd6af230801b..771df0f670fc00 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,94 @@ 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). + // Fall through and return whatever we have so far. } 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 00000000000000..a90536c55c947d --- /dev/null +++ b/test/parallel/test-assert-long-line-perf.js @@ -0,0 +1,61 @@ +'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 does not hang on long single-line source', () => { + // Generate a synthetic minified file: many variable declarations on a + // single line followed by a failing assert.ok call. + let code = ''; + for (let i = 0; i < 100000; i++) { + code += `var a${i}=function(){return ${i}};`; + } + code += "require('node:assert').ok(false)"; + + const file = path.join(tmpdir.path, 'assert-minified-perf.js'); + fs.writeFileSync(file, code); + + // Run the file as a child process with a generous timeout. + // Before the fix, this would hang for minutes. After the fix, it should + // complete in well under 5 seconds even on slow CI machines. + const result = spawnSync(process.execPath, [file], { + timeout: 10_000, + encoding: 'utf8', + }); + + // 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/); +}); + +test('assert.ok error message is correct for long single-line source', () => { + // A long line with a failing assert at the end should still produce + // a meaningful error message with the expression. + const padding = ';'.repeat(5000); + 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: 10_000, + encoding: 'utf8', + }); + + assert.strictEqual(result.signal, null); + assert.strictEqual(result.status, 1); + assert.match(result.stderr, /ok\(false\)/); +}); From 92796523db44da52ad5616e12210776710ad2c69 Mon Sep 17 00:00:00 2001 From: Felipe Coelho Date: Thu, 19 Mar 2026 12:30:55 -0300 Subject: [PATCH 2/2] fix: improve windowing edge cases and test reliability - Return undefined from catch block in getFirstExpression when acorn throws on malformed windowed code, instead of falling through and returning garbage from partially-tokenized input. - Reduce test fixture size from 100K variable declarations (~2.8MB) to ~3000 chars (just above kMaxSourceLineLength threshold). This still exercises the windowing path without stressing the filesystem. - Replace fixed 10s timeout as correctness assertion with elapsed time measurement via process.hrtime.bigint(). The test now asserts completion in under 10 seconds, which is more robust on slow CI. --- lib/internal/errors/error_source.js | 4 ++- test/parallel/test-assert-long-line-perf.js | 34 +++++++++++---------- 2 files changed, 21 insertions(+), 17 deletions(-) diff --git a/lib/internal/errors/error_source.js b/lib/internal/errors/error_source.js index 771df0f670fc00..acd27bdb4eb197 100644 --- a/lib/internal/errors/error_source.js +++ b/lib/internal/errors/error_source.js @@ -175,7 +175,9 @@ function getFirstExpression(code, startColumn) { } } catch { // The windowed code may be invalid JavaScript (e.g., starting mid-string-literal). - // Fall through and return whatever we have so far. + // 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 index a90536c55c947d..82a3370135a42d 100644 --- a/test/parallel/test-assert-long-line-perf.js +++ b/test/parallel/test-assert-long-line-perf.js @@ -13,25 +13,22 @@ const fs = require('fs'); const path = require('path'); const { test } = require('node:test'); -test('assert.ok does not hang on long single-line source', () => { - // Generate a synthetic minified file: many variable declarations on a - // single line followed by a failing assert.ok call. - let code = ''; - for (let i = 0; i < 100000; i++) { - code += `var a${i}=function(){return ${i}};`; - } - code += "require('node:assert').ok(false)"; +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); - // Run the file as a child process with a generous timeout. - // Before the fix, this would hang for minutes. After the fix, it should - // complete in well under 5 seconds even on slow CI machines. + const start = process.hrtime.bigint(); const result = spawnSync(process.execPath, [file], { - timeout: 10_000, + 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. @@ -39,19 +36,24 @@ test('assert.ok does not hang on long single-line source', () => { '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 a failing assert at the end should still produce - // a meaningful error message with the expression. - const padding = ';'.repeat(5000); + // 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: 10_000, + timeout: 30_000, encoding: 'utf8', });