Node: user-land modules failing to hook process exit

[bcoe]

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

In [email protected], 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.[email protected] and https://github.com/tapjs/signal-exit/blob/master/index.js

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

potential culprits I saw:

https://github.com/nodejs/node/pull/25020
https://github.com/nodejs/node/pull/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 dde71520ba7439741c3314b719ea88dc0ed8b9a7 (https://github.com/nodejs/node/pull/25127 / https://github.com/nodejs/node/pull/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! :slightly_smiling_face:)

[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?

[nicojs]

Yeah. No tool should ever use process.exit() IMHO. It's super unfriendly to others using your tool.

[bcoe]

worth mentioning that the shenanigans around capturing signals/exits was a big motivator for my wanting to move coverage into Node.js itself:

https://github.com/nodejs/node/blob/d3806f9f3cded6bce9831f5d8ff88372ba7e5861/lib/internal/bootstrap/node.js#L312

☝️ensuring that events are handled last in a user-land module becomes an arm wrestling contest with whatever other user-land modules are running, e.g., the interaction between nyc/mocha. I wonder if a better API in the future might better represent the fact that there might be a stack of exit events?

[addaleax]

@bcoe I think this is really something to be fixed in mocha here. process.on('exit') is a pretty good API imo, and maybe the biggest deficiency here is that we do not explicitly discourage using process.exit() inside it.

I’ve opened https://github.com/mochajs/mocha/pull/3684 to addres this in mocha.