Skip to content

Commit

Permalink
process: fix process.exitCode handling for fatalException
Browse files Browse the repository at this point in the history
* set process.exitCode before calling 'exit' handlers so that there will
  not be a situation where process.exitCode !== code in 'exit' callback
  during uncaughtException handling
* don't ignore process.exitCode set in 'exit' callback when failed with
  uncaughtException and there is no uncaughtException listener

PR-URL: #21739
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
  • Loading branch information
lundibundi authored and targos committed Jul 14, 2018
1 parent aa5994f commit 961f6e8
Show file tree
Hide file tree
Showing 7 changed files with 109 additions and 8 deletions.
8 changes: 6 additions & 2 deletions doc/api/process.md
Original file line number Diff line number Diff line change
Expand Up @@ -151,9 +151,13 @@ added: v0.1.18

The `'uncaughtException'` event is emitted when an uncaught JavaScript
exception bubbles all the way back to the event loop. By default, Node.js
handles such exceptions by printing the stack trace to `stderr` and exiting.
handles such exceptions by printing the stack trace to `stderr` and exiting
with code 1, overriding any previously set [`process.exitCode`][].
Adding a handler for the `'uncaughtException'` event overrides this default
behavior.
behavior. You may also change the [`process.exitCode`][] in
`'uncaughtException'` handler which will result in process exiting with
provided exit code, otherwise in the presence of such handler the process will
exit with 0.

The listener function is called with the `Error` object passed as the only
argument.
Expand Down
1 change: 1 addition & 0 deletions lib/internal/bootstrap/node.js
Original file line number Diff line number Diff line change
Expand Up @@ -449,6 +449,7 @@
try {
if (!process._exiting) {
process._exiting = true;
process.exitCode = 1;
process.emit('exit', 1);
}
} catch {
Expand Down
3 changes: 0 additions & 3 deletions lib/internal/worker.js
Original file line number Diff line number Diff line change
Expand Up @@ -454,9 +454,6 @@ function setupChild(evalScript) {
debug(`[${threadId}] fatal exception caught = ${caught}`);

if (!caught) {
// set correct code (uncaughtException) for [kOnExit](code) handler
process.exitCode = 1;

let serialized;
try {
serialized = serializeError(error);
Expand Down
11 changes: 10 additions & 1 deletion src/node.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1952,7 +1952,16 @@ void FatalException(Isolate* isolate,
exit(7);
} else if (caught->IsFalse()) {
ReportException(env, error, message);
exit(1);

// fatal_exception_function call before may have set a new exit code ->
// read it again, otherwise use default for uncaughtException 1
Local<String> exit_code = env->exit_code_string();
Local<Value> code;
if (!process_object->Get(env->context(), exit_code).ToLocal(&code) ||
!code->IsInt32()) {
exit(1);
}
exit(code.As<v8::Int32>()->Value());
}
}
}
Expand Down
56 changes: 55 additions & 1 deletion test/parallel/test-process-exit-code.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,14 @@ switch (process.argv[2]) {
return child4();
case 'child5':
return child5();
case 'child6':
return child6();
case 'child7':
return child7();
case 'child8':
return child8();
case 'child9':
return child9();
case undefined:
return parent();
default:
Expand All @@ -43,13 +51,15 @@ switch (process.argv[2]) {
function child1() {
process.exitCode = 42;
process.on('exit', function(code) {
assert.strictEqual(process.exitCode, 42);
assert.strictEqual(code, 42);
});
}

function child2() {
process.exitCode = 99;
process.on('exit', function(code) {
assert.strictEqual(process.exitCode, 42);
assert.strictEqual(code, 42);
});
process.exit(42);
Expand All @@ -58,6 +68,7 @@ function child2() {
function child3() {
process.exitCode = 99;
process.on('exit', function(code) {
assert.strictEqual(process.exitCode, 0);
assert.strictEqual(code, 0);
});
process.exit(0);
Expand All @@ -66,7 +77,7 @@ function child3() {
function child4() {
process.exitCode = 99;
process.on('exit', function(code) {
if (code !== 1) {
if (code !== 1 || process.exitCode !== 1) {
console.log('wrong code! expected 1 for uncaughtException');
process.exit(99);
}
Expand All @@ -77,11 +88,50 @@ function child4() {
function child5() {
process.exitCode = 95;
process.on('exit', function(code) {
assert.strictEqual(process.exitCode, 95);
assert.strictEqual(code, 95);
process.exitCode = 99;
});
}

function child6() {
process.on('exit', function(code) {
assert.strictEqual(process.exitCode, 0);
assert.strictEqual(code, 0);
});
process.on('uncaughtException', () => {});
throw new Error('ok');
}

function child7() {
process.on('exit', function(code) {
assert.strictEqual(process.exitCode, 97);
assert.strictEqual(code, 97);
});
process.on('uncaughtException', () => {
process.exitCode = 97;
});
throw new Error('ok');
}

function child8() {
process.on('exit', function(code) {
assert.strictEqual(process.exitCode, 1);
assert.strictEqual(code, 1);
process.exitCode = 98;
});
throw new Error('ok');
}

function child9() {
process.on('exit', function(code) {
assert.strictEqual(process.exitCode, 1);
assert.strictEqual(code, 1);
process.exitCode = 0;
});
throw new Error('ok');
}

function parent() {
const { spawn } = require('child_process');
const node = process.execPath;
Expand All @@ -102,4 +152,8 @@ function parent() {
test('child3', 0);
test('child4', 1);
test('child5', 99);
test('child6', 0);
test('child7', 97);
test('child8', 98);
test('child9', 0);
}
28 changes: 27 additions & 1 deletion test/parallel/test-worker-exit-code.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,10 @@ if (!process.env.HAS_STARTED_WORKER) {
return child6();
case 'child7':
return child7();
case 'child8':
return child8();
case 'child9':
return child9();
default:
throw new Error('invalid');
}
Expand Down Expand Up @@ -105,6 +109,24 @@ function child7() {
throw new Error('ok');
}

function child8() {
process.on('exit', (code) => {
assert.strictEqual(process.exitCode, 1);
assert.strictEqual(code, 1);
process.exitCode = 98;
});
throw new Error('ok');
}

function child9() {
process.on('exit', function(code) {
assert.strictEqual(process.exitCode, 1);
assert.strictEqual(code, 1);
process.exitCode = 0;
});
throw new Error('ok');
}

function parent() {
const test = (arg, exit, error = null) => {
const w = new Worker(__filename);
Expand All @@ -116,7 +138,9 @@ function parent() {
}));
if (error) {
w.on('error', common.mustCall((err) => {
assert(error.test(err));
console.log(err);
assert(error.test(err),
`wrong error for ${arg}\nexpected:${error} but got:${err}`);
}));
}
w.postMessage(arg);
Expand All @@ -129,4 +153,6 @@ function parent() {
test('child5', 99);
test('child6', 0);
test('child7', 97);
test('child8', 98, /^Error: ok$/);
test('child9', 0, /^Error: ok$/);
}
10 changes: 10 additions & 0 deletions test/parallel/test-worker-uncaught-exception.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,22 @@ if (!process.env.HAS_STARTED_WORKER) {
const w = new Worker(__filename);
w.on('message', common.mustNotCall());
w.on('error', common.mustCall((err) => {
console.log(err.message);
assert(/^Error: foo$/.test(err));
}));
w.on('exit', common.mustCall((code) => {
// uncaughtException is code 1
assert.strictEqual(code, 1);
}));
} else {
// cannot use common.mustCall as it cannot catch this
let called = false;
process.on('exit', (code) => {
if (!called) {
called = true;
} else {
assert.fail('Exit callback called twice in worker');
}
});
throw new Error('foo');
}

0 comments on commit 961f6e8

Please sign in to comment.