fix: handle signals more correctly (#142)

previously we were assuming that npm received the signal first and was
required to forward the signal to child processes, however that seems to
no longer be the case. instead npm and the child process both receive
the signal at the same time. the previous logic has been modified such
that it places a no-op function as the signal handler. this is strictly
to prevent the default behavior of exiting node from happening. once all
child process have exited, the handlers are all removed and we exit
appropriately

Author: nlf

lib/run-script-pkg.js

@@ -94,7 +94,11 @@ const runScriptPkg = async options => {
   return p.catch(er => {
     const { signal } = er
     if (stdio === 'inherit' && signal) {
+      // by the time we reach here, the child has already exited. we send the
+      // signal back to ourselves again so that npm will exit with the same
+      // status as the child
       process.kill(process.pid, signal)
+
       // just in case we don't die, reject after 500ms
       // this also keeps the node process open long enough to actually
       // get the signal, rather than terminating gracefully.

lib/signal-manager.js

@@ -1,17 +1,19 @@
 const runningProcs = new Set()
 let handlersInstalled = false
 
+// NOTE: these signals aren't actually forwarded anywhere. they're trapped and
+// ignored until all child processes have exited. in our next breaking change
+// we should rename this
 const forwardedSignals = [
   'SIGINT',
   'SIGTERM',
 ]
 
-const handleSignal = signal => {
-  for (const proc of runningProcs) {
-    proc.kill(signal)
-  }
-}
-
+// no-op, this is so receiving the signal doesn't cause us to exit immediately
+// instead, we exit after all children have exited when we re-send the signal
+// to ourselves. see the catch handler at the bottom of run-script-pkg.js
+// istanbul ignore next - this function does nothing
+const handleSignal = () => {}
 const setupListeners = () => {
   for (const signal of forwardedSignals) {
     process.on(signal, handleSignal)

test/signal-manager.js

@@ -44,24 +44,3 @@ test('adds only one handler for each signal, removes handlers when children have
 
   t.end()
 })
-
-test('forwards signals to child process', t => {
-  const proc = new EventEmitter()
-  proc.kill = (signal) => {
-    t.equal(signal, signalManager.forwardedSignals[0], 'child receives correct signal')
-    proc.emit('exit', 0)
-    for (const forwarded of signalManager.forwardedSignals) {
-      t.equal(
-        process.listeners(forwarded).includes(signalManager.handleSignal),
-        false, 'listener has been removed')
-    }
-    t.end()
-  }
-
-  signalManager.add(proc)
-  // passing the signal name here is necessary to fake the effects of actually
-  // receiving the signal per nodejs documentation signal handlers receive the
-  // name of the signal as their first parameter
-  // https://nodejs.org/api/process.html#process_signal_events
-  process.emit(signalManager.forwardedSignals[0], signalManager.forwardedSignals[0])
-})