From 54c8339e94e3fe96f71737754cc68b5dc1f5d02d Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Wed, 20 Jul 2022 19:17:43 +0800 Subject: [PATCH] bootstrap: clean up warning setup during serialization PR-URL: https://github.com/nodejs/node/pull/38905 Refs: https://github.com/nodejs/node/issues/35711 Reviewed-By: Chengzhong Wu Reviewed-By: Matteo Collina --- lib/internal/bootstrap/pre_execution.js | 30 ++++- lib/internal/process/warning.js | 28 ++-- test/fixtures/snapshot/warning.js | 1 + test/parallel/test-snapshot-warning.js | 166 ++++++++++++++++++++++++ 4 files changed, 214 insertions(+), 11 deletions(-) create mode 100644 test/fixtures/snapshot/warning.js create mode 100644 test/parallel/test-snapshot-warning.js diff --git a/lib/internal/bootstrap/pre_execution.js b/lib/internal/bootstrap/pre_execution.js index 823f2515f4f704..9e407dd29b99aa 100644 --- a/lib/internal/bootstrap/pre_execution.js +++ b/lib/internal/bootstrap/pre_execution.js @@ -28,6 +28,11 @@ const { } = require('internal/errors').codes; const assert = require('internal/assert'); +const { + addSerializeCallback, + isBuildingSnapshot, +} = require('v8').startupSnapshot; + function prepareMainThreadExecution(expandArgv1 = false, initialzeModules = true) { refreshRuntimeOptions(); @@ -169,11 +174,21 @@ function addReadOnlyProcessAlias(name, option, enumerable = true) { function setupWarningHandler() { const { - onWarning + onWarning, + resetForSerialization } = require('internal/process/warning'); if (getOptionValue('--warnings') && process.env.NODE_NO_WARNINGS !== '1') { process.on('warning', onWarning); + + // The code above would add the listener back during deserialization, + // if applicable. + if (isBuildingSnapshot()) { + addSerializeCallback(() => { + process.removeListener('warning', onWarning); + resetForSerialization(); + }); + } } } @@ -327,9 +342,18 @@ function initializeHeapSnapshotSignalHandlers() { require('internal/validators').validateSignalName(signal); const { writeHeapSnapshot } = require('v8'); - process.on(signal, () => { + function doWriteHeapSnapshot() { writeHeapSnapshot(); - }); + } + process.on(signal, doWriteHeapSnapshot); + + // The code above would add the listener back during deserialization, + // if applicable. + if (isBuildingSnapshot()) { + addSerializeCallback(() => { + process.removeListener(signal, doWriteHeapSnapshot); + }); + } } function setupTraceCategoryState() { diff --git a/lib/internal/process/warning.js b/lib/internal/process/warning.js index 9778990630dcaa..9c2a98af058957 100644 --- a/lib/internal/process/warning.js +++ b/lib/internal/process/warning.js @@ -22,6 +22,16 @@ let fs; let fd; let warningFile; let options; +let traceWarningHelperShown = false; + +function resetForSerialization() { + if (fd !== undefined) { + process.removeListener('exit', closeFdOnExit); + } + fd = undefined; + warningFile = undefined; + traceWarningHelperShown = false; +} function lazyOption() { // This will load `warningFile` only once. If the flag is not set, @@ -50,6 +60,14 @@ function writeOut(message) { error(message); } +function closeFdOnExit() { + try { + fs.closeSync(fd); + } catch { + // Continue regardless of error. + } +} + function writeToFile(message) { if (fd === undefined) { fs = require('fs'); @@ -58,13 +76,7 @@ function writeToFile(message) { } catch { return writeOut(message); } - process.on('exit', () => { - try { - fs.closeSync(fd); - } catch { - // Continue regardless of error. - } - }); + process.on('exit', closeFdOnExit); } fs.appendFile(fd, `${message}\n`, (err) => { if (err) { @@ -77,7 +89,6 @@ function doEmitWarning(warning) { process.emit('warning', warning); } -let traceWarningHelperShown = false; function onWarning(warning) { if (!(warning instanceof Error)) return; const isDeprecation = warning.name === 'DeprecationWarning'; @@ -179,4 +190,5 @@ module.exports = { emitWarning, emitWarningSync, onWarning, + resetForSerialization, }; diff --git a/test/fixtures/snapshot/warning.js b/test/fixtures/snapshot/warning.js new file mode 100644 index 00000000000000..78c39557e0471d --- /dev/null +++ b/test/fixtures/snapshot/warning.js @@ -0,0 +1 @@ +process.emitWarning('test warning'); \ No newline at end of file diff --git a/test/parallel/test-snapshot-warning.js b/test/parallel/test-snapshot-warning.js new file mode 100644 index 00000000000000..f7defe1d5b3bf4 --- /dev/null +++ b/test/parallel/test-snapshot-warning.js @@ -0,0 +1,166 @@ +'use strict'; + +// This tests that the warning handler is cleaned up properly +// during snapshot serialization and installed again during +// deserialization. + +const common = require('../common'); + +if (process.features.debug) { + common.skip('V8 snapshot does not work with mutated globals yet: ' + + 'https://bugs.chromium.org/p/v8/issues/detail?id=12772'); +} + +const assert = require('assert'); +const { spawnSync } = require('child_process'); +const tmpdir = require('../common/tmpdir'); +const fixtures = require('../common/fixtures'); +const path = require('path'); +const fs = require('fs'); + +const warningScript = fixtures.path('snapshot', 'warning.js'); +const blobPath = path.join(tmpdir.path, 'snapshot.blob'); +const empty = fixtures.path('empty.js'); + +tmpdir.refresh(); +{ + console.log('\n# Check snapshot scripts that do not emit warnings.'); + let child = spawnSync(process.execPath, [ + '--snapshot-blob', + blobPath, + '--build-snapshot', + empty, + ], { + cwd: tmpdir.path + }); + console.log('[stderr]:', child.stderr.toString()); + console.log('[stdout]:', child.stdout.toString()); + if (child.status !== 0) { + console.log(child.signal); + assert.strictEqual(child.status, 0); + } + const stats = fs.statSync(blobPath); + assert(stats.isFile()); + + child = spawnSync(process.execPath, [ + '--snapshot-blob', + blobPath, + warningScript, + ], { + cwd: tmpdir.path + }); + console.log('[stderr]:', child.stderr.toString()); + console.log('[stdout]:', child.stdout.toString()); + if (child.status !== 0) { + console.log(child.signal); + assert.strictEqual(child.status, 0); + } + const match = child.stderr.toString().match(/Warning: test warning/g); + assert.strictEqual(match.length, 1); +} + +tmpdir.refresh(); +{ + console.log('\n# Check snapshot scripts that emit ' + + 'warnings and --trace-warnings hint.'); + let child = spawnSync(process.execPath, [ + '--snapshot-blob', + blobPath, + '--build-snapshot', + warningScript, + ], { + cwd: tmpdir.path + }); + console.log('[stderr]:', child.stderr.toString()); + console.log('[stdout]:', child.stdout.toString()); + if (child.status !== 0) { + console.log(child.signal); + assert.strictEqual(child.status, 0); + } + const stats = fs.statSync(blobPath); + assert(stats.isFile()); + let match = child.stderr.toString().match(/Warning: test warning/g); + assert.strictEqual(match.length, 1); + match = child.stderr.toString().match(/Use `node --trace-warnings/g); + assert.strictEqual(match.length, 1); + + child = spawnSync(process.execPath, [ + '--snapshot-blob', + blobPath, + warningScript, + ], { + cwd: tmpdir.path + }); + console.log('[stderr]:', child.stderr.toString()); + console.log('[stdout]:', child.stdout.toString()); + if (child.status !== 0) { + console.log(child.signal); + assert.strictEqual(child.status, 0); + } + // Warnings should not be handled more than once. + match = child.stderr.toString().match(/Warning: test warning/g); + assert.strictEqual(match.length, 1); + match = child.stderr.toString().match(/Use `node --trace-warnings/g); + assert.strictEqual(match.length, 1); +} + +tmpdir.refresh(); +{ + console.log('\n# Check --redirect-warnings'); + const warningFile1 = path.join(tmpdir.path, 'warnings.txt'); + const warningFile2 = path.join(tmpdir.path, 'warnings2.txt'); + + let child = spawnSync(process.execPath, [ + '--snapshot-blob', + blobPath, + '--redirect-warnings', + warningFile1, + '--build-snapshot', + warningScript, + ], { + cwd: tmpdir.path + }); + console.log('[stderr]:', child.stderr.toString()); + console.log('[stdout]:', child.stdout.toString()); + if (child.status !== 0) { + console.log(child.signal); + assert.strictEqual(child.status, 0); + } + const stats = fs.statSync(blobPath); + assert(stats.isFile()); + const warnings1 = fs.readFileSync(warningFile1, 'utf8'); + console.log(warningFile1, ':', warnings1); + let match = warnings1.match(/Warning: test warning/g); + assert.strictEqual(match.length, 1); + match = warnings1.match(/Use `node --trace-warnings/g); + assert.strictEqual(match.length, 1); + assert.doesNotMatch(child.stderr.toString(), /Warning: test warning/); + + fs.rmSync(warningFile1, { + maxRetries: 3, recursive: false, force: true + }); + child = spawnSync(process.execPath, [ + '--snapshot-blob', + blobPath, + '--redirect-warnings', + warningFile2, + warningScript, + ], { + cwd: tmpdir.path + }); + console.log('[stderr]:', child.stderr.toString()); + console.log('[stdout]:', child.stdout.toString()); + if (child.status !== 0) { + console.log(child.signal); + assert.strictEqual(child.status, 0); + } + assert(!fs.existsSync(warningFile1)); + + const warnings2 = fs.readFileSync(warningFile2, 'utf8'); + console.log(warningFile2, ':', warnings1); + match = warnings2.match(/Warning: test warning/g); + assert.strictEqual(match.length, 1); + match = warnings2.match(/Use `node --trace-warnings/g); + assert.strictEqual(match.length, 1); + assert.doesNotMatch(child.stderr.toString(), /Warning: test warning/); +}