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 PR-URL: #21739 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
nodejs · Jul 14, 2018 · 961f6e8 · 961f6e8
1 parent aa5994f
commit 961f6e8
Show file tree

Hide file tree

Showing 7 changed files with 109 additions and 8 deletions.
diff --git 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
@@ -449,6 +449,7 @@
         try {
           if (!process._exiting) {
             process._exiting = true;
+            process.exitCode = 1;
             process.emit('exit', 1);
           }
         } catch {

diff --git 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);

diff --git a/src/node.cc b/src/node.cc
@@ -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());
     }
   }
 }

diff --git 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,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);
@@ -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
@@ -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
@@ -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');
 }