bootstrap: clean up warning setup during serialization

PR-URL: nodejs#38905 Refs: nodejs#35711 Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
Fyko · Sep 15, 2022 · aa44fe5 · aa44fe5
1 parent 53eb96d
commit aa44fe5
Show file tree

Hide file tree

Showing 4 changed files with 214 additions and 11 deletions.
diff --git 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
@@ -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
@@ -0,0 +1 @@
+process.emitWarning('test warning');
diff --git a/test/parallel/test-snapshot-warning.js 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/);
+}