Skip to content

Commit 54c8339

Browse files
joyeecheungdanielleadams
joyeecheung
authored and
danielleadams
committed
bootstrap: clean up warning setup during serialization
PR-URL: #38905 Refs: #35711 Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
1 parent 31589b5 commit 54c8339

File tree

4 files changed

+214
-11
lines changed

4 files changed

+214
-11
lines changed

‎lib/internal/bootstrap/pre_execution.js

+27-3
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,11 @@ const {
2828
} = require('internal/errors').codes;
2929
const assert = require('internal/assert');
3030

31+
const {
32+
addSerializeCallback,
33+
isBuildingSnapshot,
34+
} = require('v8').startupSnapshot;
35+
3136
function prepareMainThreadExecution(expandArgv1 = false,
3237
initialzeModules = true) {
3338
refreshRuntimeOptions();
@@ -169,11 +174,21 @@ function addReadOnlyProcessAlias(name, option, enumerable = true) {
169174

170175
function setupWarningHandler() {
171176
const {
172-
onWarning
177+
onWarning,
178+
resetForSerialization
173179
} = require('internal/process/warning');
174180
if (getOptionValue('--warnings') &&
175181
process.env.NODE_NO_WARNINGS !== '1') {
176182
process.on('warning', onWarning);
183+
184+
// The code above would add the listener back during deserialization,
185+
// if applicable.
186+
if (isBuildingSnapshot()) {
187+
addSerializeCallback(() => {
188+
process.removeListener('warning', onWarning);
189+
resetForSerialization();
190+
});
191+
}
177192
}
178193
}
179194

@@ -327,9 +342,18 @@ function initializeHeapSnapshotSignalHandlers() {
327342
require('internal/validators').validateSignalName(signal);
328343
const { writeHeapSnapshot } = require('v8');
329344

330-
process.on(signal, () => {
345+
function doWriteHeapSnapshot() {
331346
writeHeapSnapshot();
332-
});
347+
}
348+
process.on(signal, doWriteHeapSnapshot);
349+
350+
// The code above would add the listener back during deserialization,
351+
// if applicable.
352+
if (isBuildingSnapshot()) {
353+
addSerializeCallback(() => {
354+
process.removeListener(signal, doWriteHeapSnapshot);
355+
});
356+
}
333357
}
334358

335359
function setupTraceCategoryState() {

‎lib/internal/process/warning.js

+20-8
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,16 @@ let fs;
2222
let fd;
2323
let warningFile;
2424
let options;
25+
let traceWarningHelperShown = false;
26+
27+
function resetForSerialization() {
28+
if (fd !== undefined) {
29+
process.removeListener('exit', closeFdOnExit);
30+
}
31+
fd = undefined;
32+
warningFile = undefined;
33+
traceWarningHelperShown = false;
34+
}
2535

2636
function lazyOption() {
2737
// This will load `warningFile` only once. If the flag is not set,
@@ -50,6 +60,14 @@ function writeOut(message) {
5060
error(message);
5161
}
5262

63+
function closeFdOnExit() {
64+
try {
65+
fs.closeSync(fd);
66+
} catch {
67+
// Continue regardless of error.
68+
}
69+
}
70+
5371
function writeToFile(message) {
5472
if (fd === undefined) {
5573
fs = require('fs');
@@ -58,13 +76,7 @@ function writeToFile(message) {
5876
} catch {
5977
return writeOut(message);
6078
}
61-
process.on('exit', () => {
62-
try {
63-
fs.closeSync(fd);
64-
} catch {
65-
// Continue regardless of error.
66-
}
67-
});
79+
process.on('exit', closeFdOnExit);
6880
}
6981
fs.appendFile(fd, `${message}\n`, (err) => {
7082
if (err) {
@@ -77,7 +89,6 @@ function doEmitWarning(warning) {
7789
process.emit('warning', warning);
7890
}
7991

80-
let traceWarningHelperShown = false;
8192
function onWarning(warning) {
8293
if (!(warning instanceof Error)) return;
8394
const isDeprecation = warning.name === 'DeprecationWarning';
@@ -179,4 +190,5 @@ module.exports = {
179190
emitWarning,
180191
emitWarningSync,
181192
onWarning,
193+
resetForSerialization,
182194
};

‎test/fixtures/snapshot/warning.js

+1
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
process.emitWarning('test warning');

‎test/parallel/test-snapshot-warning.js

+166
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,166 @@
1+
'use strict';
2+
3+
// This tests that the warning handler is cleaned up properly
4+
// during snapshot serialization and installed again during
5+
// deserialization.
6+
7+
const common = require('../common');
8+
9+
if (process.features.debug) {
10+
common.skip('V8 snapshot does not work with mutated globals yet: ' +
11+
'https://bugs.chromium.org/p/v8/issues/detail?id=12772');
12+
}
13+
14+
const assert = require('assert');
15+
const { spawnSync } = require('child_process');
16+
const tmpdir = require('../common/tmpdir');
17+
const fixtures = require('../common/fixtures');
18+
const path = require('path');
19+
const fs = require('fs');
20+
21+
const warningScript = fixtures.path('snapshot', 'warning.js');
22+
const blobPath = path.join(tmpdir.path, 'snapshot.blob');
23+
const empty = fixtures.path('empty.js');
24+
25+
tmpdir.refresh();
26+
{
27+
console.log('\n# Check snapshot scripts that do not emit warnings.');
28+
let child = spawnSync(process.execPath, [
29+
'--snapshot-blob',
30+
blobPath,
31+
'--build-snapshot',
32+
empty,
33+
], {
34+
cwd: tmpdir.path
35+
});
36+
console.log('[stderr]:', child.stderr.toString());
37+
console.log('[stdout]:', child.stdout.toString());
38+
if (child.status !== 0) {
39+
console.log(child.signal);
40+
assert.strictEqual(child.status, 0);
41+
}
42+
const stats = fs.statSync(blobPath);
43+
assert(stats.isFile());
44+
45+
child = spawnSync(process.execPath, [
46+
'--snapshot-blob',
47+
blobPath,
48+
warningScript,
49+
], {
50+
cwd: tmpdir.path
51+
});
52+
console.log('[stderr]:', child.stderr.toString());
53+
console.log('[stdout]:', child.stdout.toString());
54+
if (child.status !== 0) {
55+
console.log(child.signal);
56+
assert.strictEqual(child.status, 0);
57+
}
58+
const match = child.stderr.toString().match(/Warning: test warning/g);
59+
assert.strictEqual(match.length, 1);
60+
}
61+
62+
tmpdir.refresh();
63+
{
64+
console.log('\n# Check snapshot scripts that emit ' +
65+
'warnings and --trace-warnings hint.');
66+
let child = spawnSync(process.execPath, [
67+
'--snapshot-blob',
68+
blobPath,
69+
'--build-snapshot',
70+
warningScript,
71+
], {
72+
cwd: tmpdir.path
73+
});
74+
console.log('[stderr]:', child.stderr.toString());
75+
console.log('[stdout]:', child.stdout.toString());
76+
if (child.status !== 0) {
77+
console.log(child.signal);
78+
assert.strictEqual(child.status, 0);
79+
}
80+
const stats = fs.statSync(blobPath);
81+
assert(stats.isFile());
82+
let match = child.stderr.toString().match(/Warning: test warning/g);
83+
assert.strictEqual(match.length, 1);
84+
match = child.stderr.toString().match(/Use `node --trace-warnings/g);
85+
assert.strictEqual(match.length, 1);
86+
87+
child = spawnSync(process.execPath, [
88+
'--snapshot-blob',
89+
blobPath,
90+
warningScript,
91+
], {
92+
cwd: tmpdir.path
93+
});
94+
console.log('[stderr]:', child.stderr.toString());
95+
console.log('[stdout]:', child.stdout.toString());
96+
if (child.status !== 0) {
97+
console.log(child.signal);
98+
assert.strictEqual(child.status, 0);
99+
}
100+
// Warnings should not be handled more than once.
101+
match = child.stderr.toString().match(/Warning: test warning/g);
102+
assert.strictEqual(match.length, 1);
103+
match = child.stderr.toString().match(/Use `node --trace-warnings/g);
104+
assert.strictEqual(match.length, 1);
105+
}
106+
107+
tmpdir.refresh();
108+
{
109+
console.log('\n# Check --redirect-warnings');
110+
const warningFile1 = path.join(tmpdir.path, 'warnings.txt');
111+
const warningFile2 = path.join(tmpdir.path, 'warnings2.txt');
112+
113+
let child = spawnSync(process.execPath, [
114+
'--snapshot-blob',
115+
blobPath,
116+
'--redirect-warnings',
117+
warningFile1,
118+
'--build-snapshot',
119+
warningScript,
120+
], {
121+
cwd: tmpdir.path
122+
});
123+
console.log('[stderr]:', child.stderr.toString());
124+
console.log('[stdout]:', child.stdout.toString());
125+
if (child.status !== 0) {
126+
console.log(child.signal);
127+
assert.strictEqual(child.status, 0);
128+
}
129+
const stats = fs.statSync(blobPath);
130+
assert(stats.isFile());
131+
const warnings1 = fs.readFileSync(warningFile1, 'utf8');
132+
console.log(warningFile1, ':', warnings1);
133+
let match = warnings1.match(/Warning: test warning/g);
134+
assert.strictEqual(match.length, 1);
135+
match = warnings1.match(/Use `node --trace-warnings/g);
136+
assert.strictEqual(match.length, 1);
137+
assert.doesNotMatch(child.stderr.toString(), /Warning: test warning/);
138+
139+
fs.rmSync(warningFile1, {
140+
maxRetries: 3, recursive: false, force: true
141+
});
142+
child = spawnSync(process.execPath, [
143+
'--snapshot-blob',
144+
blobPath,
145+
'--redirect-warnings',
146+
warningFile2,
147+
warningScript,
148+
], {
149+
cwd: tmpdir.path
150+
});
151+
console.log('[stderr]:', child.stderr.toString());
152+
console.log('[stdout]:', child.stdout.toString());
153+
if (child.status !== 0) {
154+
console.log(child.signal);
155+
assert.strictEqual(child.status, 0);
156+
}
157+
assert(!fs.existsSync(warningFile1));
158+
159+
const warnings2 = fs.readFileSync(warningFile2, 'utf8');
160+
console.log(warningFile2, ':', warnings1);
161+
match = warnings2.match(/Warning: test warning/g);
162+
assert.strictEqual(match.length, 1);
163+
match = warnings2.match(/Use `node --trace-warnings/g);
164+
assert.strictEqual(match.length, 1);
165+
assert.doesNotMatch(child.stderr.toString(), /Warning: test warning/);
166+
}

0 commit comments

Comments
 (0)