user-land modules failing to hook process exit

[bcoe]

• Version 11.7.0:
• Platform OSX:
• Subsystem process:

In Node@11.7.0, an incompatibility has been introduced such that mocha and nyc no longer work in conjunction.

here's a minimal reproduction of the bug:

https://github.com/bcoe/node-25650-bug

a working theory is that multiple exit hooks no longer play nicely together.

[bcoe]

CC: @nodejs/process @nodejs/child_process

this regression is going to cause a lot of issues to be open on Mocha/nyc so we'll probably want to dig into it pretty quickly.

[bcoe]

CC: @coreyfarrell

[bcoe]

digging a little bit deeper into the problem, the issue seems to be that registering multiple exit listeners no longer stacks. If I comment out the following lines in bin/_mocha:

/**
 * Exits Mocha when tests + code under test has finished execution (default)
 * @param {number} code - Exit code; typically # of failures
 */
const exitLater = code => {
  /*process.on('exit', () => {
    process.exit(Math.min(code, 255));
  });*/
};

coverage starts working again (because nyc again gets exit events).

The ultimate root cause will likely be an interaction between a patch in Node.js@11.7.0 and https://github.com/tapjs/signal-exit/blob/master/index.js

Any thoughts on this one? @isaacs @addaleax @joyeecheung?

potential culprits I saw:

#25020
#25020

but no obvious smoking gun.

[addaleax]

#25020 doesn’t look like a likely candidate to me, it shouldn’t change any user-visible behaviour.

And, maybe for context, we have this test which is supposed to check how process.exit() inside process.on('exit') behaves: https://github.com/nodejs/node/blob/1ef175e5a434cd8c8f5bf42cf39df242c07aefd3/test/parallel/test-process-exit-recursive.js

[addaleax]

git bisect says it’s dde7152 (#25127 / #25496).

I think overriding process.reallyExit was broken in the process?

$ nvm use 11.7
Now using node v11.7.0 (npm v6.5.0)
$ node -e 'process.reallyExit = function() { console.log("reallyExit") } ; process.exit()'
$ nvm use 11.6
Now using node v11.6.0 (npm v6.5.0-next.0)
$ node -e 'process.reallyExit = function() { console.log("reallyExit") } ; process.exit()'
reallyExit

[addaleax]

This seems to be enough to fix it:

diff --git a/lib/internal/process/per_thread.js b/lib/internal/process/per_thread.js
index 6d64c636b244..eba1a21cfa02 100644
--- a/lib/internal/process/per_thread.js
+++ b/lib/internal/process/per_thread.js
@@ -149,7 +149,7 @@ function wrapProcessMethods(binding) {
       process._exiting = true;
       process.emit('exit', process.exitCode || 0);
     }
-    binding.reallyExit(process.exitCode || 0);
+    process.reallyExit(process.exitCode || 0);
   }
 
   function kill(pid, sig) {

(If anybody wants to double-check + turn this into a PR, go for it – I’m going to sleep! 🙂)

[bcoe]

@addaleax great work! I need to get better at using git bisect.

[addaleax]

Fwiw: I think the underlying issue is that mocha calls process.exit() inside process.on('exit'). They just shouldn’t be doing that, and I think what they really want is to change process.exitCode instead of calling that function?