From 18e6b2c157619416bf4b54d42a51aac97474566f Mon Sep 17 00:00:00 2001 From: Denys Otrishko Date: Tue, 10 Jul 2018 18:44:16 +0300 Subject: [PATCH 1/4] process: fix process.exitCode handling for fatalException * 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 --- doc/api/process.md | 8 ++- lib/internal/bootstrap/node.js | 1 + src/node.cc | 11 +++- test/parallel/test-process-exit-code.js | 56 ++++++++++++++++++- test/parallel/test-worker-exit-code.js | 28 +++++++++- .../test-worker-uncaught-exception.js | 10 ++++ 6 files changed, 109 insertions(+), 5 deletions(-) diff --git a/doc/api/process.md b/doc/api/process.md index edf01258c3941b..df544e7f0f17b9 100644 --- a/doc/api/process.md +++ b/doc/api/process.md @@ -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. diff --git a/lib/internal/bootstrap/node.js b/lib/internal/bootstrap/node.js index e433c67aa42c8b..016c0c5e2304bc 100644 --- a/lib/internal/bootstrap/node.js +++ b/lib/internal/bootstrap/node.js @@ -475,6 +475,7 @@ try { if (!process._exiting) { process._exiting = true; + process.exitCode = 1; process.emit('exit', 1); } } catch { diff --git a/src/node.cc b/src/node.cc index f9ac48c0770221..f7eae49bd0e5c2 100644 --- a/src/node.cc +++ b/src/node.cc @@ -1438,7 +1438,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 exit_code = env->exit_code_string(); + Local code; + if (!process_object->Get(env->context(), exit_code).ToLocal(&code) || + !code->IsInt32()) { + exit(1); + } + exit(code.As()->Value()); } } } diff --git a/test/parallel/test-process-exit-code.js b/test/parallel/test-process-exit-code.js index f5f8099c8d2439..20004a9d7d6f58 100644 --- a/test/parallel/test-process-exit-code.js +++ b/test/parallel/test-process-exit-code.js @@ -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: @@ -43,6 +51,7 @@ switch (process.argv[2]) { function child1() { process.exitCode = 42; process.on('exit', function(code) { + assert.strictEqual(process.exitCode, 42); assert.strictEqual(code, 42); }); } @@ -50,6 +59,7 @@ function child1() { function child2() { process.exitCode = 99; process.on('exit', function(code) { + assert.strictEqual(process.exitCode, 42); assert.strictEqual(code, 42); }); process.exit(42); @@ -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); @@ -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); } @@ -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; @@ -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); } diff --git a/test/parallel/test-worker-exit-code.js b/test/parallel/test-worker-exit-code.js index b621389b49ca6b..0a1e9bb95370eb 100644 --- a/test/parallel/test-worker-exit-code.js +++ b/test/parallel/test-worker-exit-code.js @@ -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'); } @@ -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); @@ -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); @@ -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$/); } diff --git a/test/parallel/test-worker-uncaught-exception.js b/test/parallel/test-worker-uncaught-exception.js index 67b861e22619aa..8193dccbd1ff2f 100644 --- a/test/parallel/test-worker-uncaught-exception.js +++ b/test/parallel/test-worker-uncaught-exception.js @@ -10,6 +10,7 @@ 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) => { @@ -17,5 +18,14 @@ if (!process.env.HAS_STARTED_WORKER) { 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'); } From 4df2d6b70baffe337ee7d2c25735d46a40164842 Mon Sep 17 00:00:00 2001 From: Denys Otrishko Date: Wed, 11 Jul 2018 00:58:01 +0300 Subject: [PATCH 2/4] test: refactor process/worker exitCode tests --- test/fixtures/process-exit-code-cases.js | 113 +++++++++++++++++++ test/parallel/test-process-exit-code.js | 131 +++-------------------- test/parallel/test-worker-exit-code.js | 123 ++------------------- 3 files changed, 133 insertions(+), 234 deletions(-) create mode 100644 test/fixtures/process-exit-code-cases.js diff --git a/test/fixtures/process-exit-code-cases.js b/test/fixtures/process-exit-code-cases.js new file mode 100644 index 00000000000000..c8c4a2ebe6c7ff --- /dev/null +++ b/test/fixtures/process-exit-code-cases.js @@ -0,0 +1,113 @@ +'use strict'; + +const assert = require('assert'); + +const cases = []; +module.exports = cases; + +function exitsOnExitCodeSet() { + process.exitCode = 42; + process.on('exit', (code) => { + assert.strictEqual(process.exitCode, 42); + assert.strictEqual(code, 42); + }); +} +cases.push({ func: exitsOnExitCodeSet, result: 42 }); + +function changesCodeViaExit() { + process.exitCode = 99; + process.on('exit', (code) => { + assert.strictEqual(process.exitCode, 42); + assert.strictEqual(code, 42); + }); + process.exit(42); +} +cases.push({ func: changesCodeViaExit, result: 42 }); + +function changesCodeZeroExit() { + process.exitCode = 99; + process.on('exit', (code) => { + assert.strictEqual(process.exitCode, 0); + assert.strictEqual(code, 0); + }); + process.exit(0); +} +cases.push({ func: changesCodeZeroExit, result: 0 }); + +function exitWithOneOnUncaught() { + process.exitCode = 99; + process.on('exit', (code) => { + // cannot use assert because it will be uncaughtException -> 1 exit code + // that will render this test useless + if (code !== 1 || process.exitCode !== 1) { + console.log('wrong code! expected 1 for uncaughtException'); + process.exit(99); + } + }); + throw new Error('ok'); +} +cases.push({ + func: exitWithOneOnUncaught, + result: 1, + error: /^Error: ok$/, +}); + +function changeCodeInsideExit() { + process.exitCode = 95; + process.on('exit', (code) => { + assert.strictEqual(process.exitCode, 95); + assert.strictEqual(code, 95); + process.exitCode = 99; + }); +} +cases.push({ func: changeCodeInsideExit, result: 99 }); + +function zeroExitWithUncaughtHandler() { + process.on('exit', (code) => { + assert.strictEqual(process.exitCode, 0); + assert.strictEqual(code, 0); + }); + process.on('uncaughtException', () => {}); + throw new Error('ok'); +} +cases.push({ func: zeroExitWithUncaughtHandler, result: 0 }); + +function changeCodeInUncaughtHandler() { + process.on('exit', (code) => { + assert.strictEqual(process.exitCode, 97); + assert.strictEqual(code, 97); + }); + process.on('uncaughtException', () => { + process.exitCode = 97; + }); + throw new Error('ok'); +} +cases.push({ func: changeCodeInUncaughtHandler, result: 97 }); + +function changeCodeInExitWithUncaught() { + process.on('exit', (code) => { + assert.strictEqual(process.exitCode, 1); + assert.strictEqual(code, 1); + process.exitCode = 98; + }); + throw new Error('ok'); +} +cases.push({ + func: changeCodeInExitWithUncaught, + result: 98, + error: /^Error: ok$/, +}); + +function exitWithZeroInExitWithUncaught() { + process.on('exit', (code) => { + assert.strictEqual(process.exitCode, 1); + assert.strictEqual(code, 1); + process.exitCode = 0; + }); + throw new Error('ok'); +} +cases.push({ + func: exitWithZeroInExitWithUncaught, + result: 0, + error: /^Error: ok$/, +}); diff --git a/test/parallel/test-process-exit-code.js b/test/parallel/test-process-exit-code.js index 20004a9d7d6f58..9059b0b5c22487 100644 --- a/test/parallel/test-process-exit-code.js +++ b/test/parallel/test-process-exit-code.js @@ -23,113 +23,18 @@ require('../common'); const assert = require('assert'); -switch (process.argv[2]) { - case 'child1': - return child1(); - case 'child2': - return child2(); - case 'child3': - return child3(); - case 'child4': - 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: - throw new Error('invalid'); -} - -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); -} - -function child3() { - process.exitCode = 99; - process.on('exit', function(code) { - assert.strictEqual(process.exitCode, 0); - assert.strictEqual(code, 0); - }); - process.exit(0); -} - -function child4() { - process.exitCode = 99; - process.on('exit', function(code) { - if (code !== 1 || process.exitCode !== 1) { - console.log('wrong code! expected 1 for uncaughtException'); - process.exit(99); - } - }); - throw new Error('ok'); -} - -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'); -} +const testCases = require('../fixtures/process-exit-code-cases'); -function child9() { - process.on('exit', function(code) { - assert.strictEqual(process.exitCode, 1); - assert.strictEqual(code, 1); - process.exitCode = 0; - }); - throw new Error('ok'); +if (!process.argv[2]) { + parent(); +} else { + const i = parseInt(process.argv[2]); + if (Number.isNaN(i)) { + console.log('Invalid test case index'); + process.exit(100); + return; + } + testCases[i].func(); } function parent() { @@ -138,22 +43,14 @@ function parent() { const f = __filename; const option = { stdio: [ 0, 1, 'ignore' ] }; - const test = (arg, exit) => { + const test = (arg, name = 'child', exit) => { spawn(node, [f, arg], option).on('exit', (code) => { assert.strictEqual( code, exit, - `wrong exit for ${arg}\nexpected:${exit} but got:${code}`); + `wrong exit for ${arg}-${name}\nexpected:${exit} but got:${code}`); console.log(`ok - ${arg} exited with ${exit}`); }); }; - test('child1', 42); - test('child2', 42); - test('child3', 0); - test('child4', 1); - test('child5', 99); - test('child6', 0); - test('child7', 97); - test('child8', 98); - test('child9', 0); + testCases.forEach((tc, i) => test(i, tc.func.name, tc.result)); } diff --git a/test/parallel/test-worker-exit-code.js b/test/parallel/test-worker-exit-code.js index 0a1e9bb95370eb..f09b834f853b12 100644 --- a/test/parallel/test-worker-exit-code.js +++ b/test/parallel/test-worker-exit-code.js @@ -9,6 +9,8 @@ const assert = require('assert'); const worker = require('worker_threads'); const { Worker, parentPort } = worker; +const testCases = require('../fixtures/process-exit-code-cases'); + // Do not use isMainThread so that this test itself can be run inside a Worker. if (!process.env.HAS_STARTED_WORKER) { process.env.HAS_STARTED_WORKER = 1; @@ -19,121 +21,16 @@ if (!process.env.HAS_STARTED_WORKER) { process.exit(100); return; } - parentPort.once('message', (msg) => { - switch (msg) { - case 'child1': - return child1(); - case 'child2': - return child2(); - case 'child3': - return child3(); - case 'child4': - return child4(); - case 'child5': - return child5(); - case 'child6': - return child6(); - case 'child7': - return child7(); - case 'child8': - return child8(); - case 'child9': - return child9(); - default: - throw new Error('invalid'); - } - }); -} - -function child1() { - process.exitCode = 42; - process.on('exit', (code) => { - assert.strictEqual(code, 42); - }); -} - -function child2() { - process.exitCode = 99; - process.on('exit', (code) => { - assert.strictEqual(code, 42); - }); - process.exit(42); -} - -function child3() { - process.exitCode = 99; - process.on('exit', (code) => { - assert.strictEqual(code, 0); - }); - process.exit(0); -} - -function child4() { - process.exitCode = 99; - process.on('exit', (code) => { - // cannot use assert because it will be uncaughtException -> 1 exit code - // that will render this test useless - if (code !== 1) { - console.error('wrong code! expected 1 for uncaughtException'); - process.exit(99); - } - }); - throw new Error('ok'); -} - -function child5() { - process.exitCode = 95; - process.on('exit', (code) => { - assert.strictEqual(code, 95); - process.exitCode = 99; - }); -} - -function child6() { - process.on('exit', (code) => { - assert.strictEqual(code, 0); - }); - process.on('uncaughtException', common.mustCall(() => { - // handle - })); - throw new Error('ok'); -} - -function child7() { - process.on('exit', (code) => { - assert.strictEqual(code, 97); - }); - process.on('uncaughtException', common.mustCall(() => { - process.exitCode = 97; - })); - 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'); + parentPort.once('message', (msg) => testCases[msg].func()); } function parent() { - const test = (arg, exit, error = null) => { + const test = (arg, name = 'worker', exit, error = null) => { const w = new Worker(__filename); w.on('exit', common.mustCall((code) => { assert.strictEqual( code, exit, - `wrong exit for ${arg}\nexpected:${exit} but got:${code}`); + `wrong exit for ${arg}-${name}\nexpected:${exit} but got:${code}`); console.log(`ok - ${arg} exited with ${exit}`); })); if (error) { @@ -146,13 +43,5 @@ function parent() { w.postMessage(arg); }; - test('child1', 42); - test('child2', 42); - test('child3', 0); - test('child4', 1, /^Error: ok$/); - test('child5', 99); - test('child6', 0); - test('child7', 97); - test('child8', 98, /^Error: ok$/); - test('child9', 0, /^Error: ok$/); + testCases.forEach((tc, i) => test(i, tc.func.name, tc.result, tc.error)); } From 7ee6da0fe6ae7999139f85b16649291a1a7ebe53 Mon Sep 17 00:00:00 2001 From: Denys Otrishko Date: Wed, 11 Jul 2018 02:10:19 +0300 Subject: [PATCH 3/4] fixup! worker: remove setting exitCode after uncaughtException/exit Now we set it before the exit event, this allows to change the code inside the exit event (event with uncaughtException), therefore setting exitCode in worker is no longer needed. --- lib/internal/worker.js | 3 --- 1 file changed, 3 deletions(-) diff --git a/lib/internal/worker.js b/lib/internal/worker.js index 83389d204d285f..bcc864b5b8b330 100644 --- a/lib/internal/worker.js +++ b/lib/internal/worker.js @@ -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); From 6a993f178cd27e8581090affee2cfe2795a5e761 Mon Sep 17 00:00:00 2001 From: Denys Otrishko Date: Fri, 13 Jul 2018 18:45:18 +0300 Subject: [PATCH 4/4] worker: exit after uncaught exception Previously even after uncaught exception the worker would continue to execute until there is no more work to do. --- lib/internal/worker.js | 2 ++ .../test-worker-uncaught-exception-async.js | 14 ++++++++++++++ test/parallel/test-worker-uncaught-exception.js | 4 ++++ 3 files changed, 20 insertions(+) diff --git a/lib/internal/worker.js b/lib/internal/worker.js index bcc864b5b8b330..26c16f86e3e8fc 100644 --- a/lib/internal/worker.js +++ b/lib/internal/worker.js @@ -467,6 +467,8 @@ function setupChild(evalScript) { else port.postMessage({ type: messageTypes.COULD_NOT_SERIALIZE_ERROR }); clearAsyncIdStack(); + + process.exit(); } } } diff --git a/test/parallel/test-worker-uncaught-exception-async.js b/test/parallel/test-worker-uncaught-exception-async.js index f820976c11ebcd..862b1a66d619c3 100644 --- a/test/parallel/test-worker-uncaught-exception-async.js +++ b/test/parallel/test-worker-uncaught-exception-async.js @@ -10,6 +10,7 @@ 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) => { @@ -17,6 +18,19 @@ if (!process.env.HAS_STARTED_WORKER) { 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'); + } + }); + + setTimeout(() => assert.fail('Timeout executed after uncaughtException'), + 2000); + setImmediate(() => { throw new Error('foo'); }); diff --git a/test/parallel/test-worker-uncaught-exception.js b/test/parallel/test-worker-uncaught-exception.js index 8193dccbd1ff2f..95c142b6c70d64 100644 --- a/test/parallel/test-worker-uncaught-exception.js +++ b/test/parallel/test-worker-uncaught-exception.js @@ -27,5 +27,9 @@ if (!process.env.HAS_STARTED_WORKER) { assert.fail('Exit callback called twice in worker'); } }); + + setTimeout(() => assert.fail('Timeout executed after uncaughtException'), + 2000); + throw new Error('foo'); }