[Pkg-javascript-commits] [node-foreground-child] 53/69: Let the child decide if signals should be fatal

[Bastien Roucariès]

This is an automated email from the git hooks/post-receive script.

rouca pushed a commit to branch master
in repository node-foreground-child.

commit cdd8a36d881405a63d7afe6c03622f387a1bda63
Author: isaacs <i at izs.me>
Date:   Thu Dec 15 16:15:57 2016 -0800

    Let the child decide if signals should be fatal
    
    The use of signal-exit in this module was incorrect.
    
    If the parent did not set any handlers on, for example, SIGTERM, then if
    it receives a SIGTERM, it would conclude that it should be fatal.  The
    signal would be proxied on to the child, and then signal-exit would
    remove the handlers from the parent, and re-send the signal to exit
    appropriately.  In this case, if the child was listening for that signal
    and decided not to exit, then the parent's exit would be inappropriate.
    
    If the parent _had_ assigned listeners to SIGTERM, then signal-exit
    would conclude that the signal is _not_ fatal, and do nothing.  That
    also means that the signal would not be proxied to the child process at
    all!
    
    As of this patch, the parent attaches listeners to all would-be fatal
    signals for the current operating system (which signal-exit helpfully
    provides for us), but still only exits when and if the child exits.
    
    Note that a SIGKILL will always kill the parent process, _and never the
    child process_, because SIGKILL cannot be caught or proxied.  The only
    way to do this would be if Node provided a way to truly exec a process
    as the new foreground program in the same process space, without forking
    a separate child process.
---
 README.md              |  6 +++++
 index.js               | 25 ++++++++++++++---
 test/immortal-child.js | 73 ++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 101 insertions(+), 3 deletions(-)

diff --git a/README.md b/README.md
index 1c45a1c..d388f95 100644
--- a/README.md
+++ b/README.md
@@ -44,3 +44,9 @@ into a child process.  In these cases, foreground-child will not map
 the file descriptors into the child.  If file descriptors 0, 1, or 2
 are used for the IPC channel, then strange behavior may happen (like
 printing IPC messages to stderr, for example).
+
+Note that a SIGKILL will always kill the parent process, _and never
+the child process_, because SIGKILL cannot be caught or proxied.  The
+only way to do this would be if Node provided a way to truly exec a
+process as the new foreground program in the same process space,
+without forking a separate child process.
diff --git a/index.js b/index.js
index aa48c6f..837793d 100644
--- a/index.js
+++ b/index.js
@@ -39,15 +39,16 @@ module.exports = function (program, args, cb) {
   var child = spawn(program, args, spawnOpts)
 
   var childExited = false
-  signalExit(function (code, signal) {
-    child.kill(signal || 'SIGHUP')
-  })
+  var unproxySignals = proxySignals(child)
+  process.on('exit', childHangup)
 
   child.on('close', function (code, signal) {
     // Allow the callback to inspect the child’s exit code and/or modify it.
     process.exitCode = signal ? 128 + signal : code
 
     cb(function () {
+      unproxySignals()
+      process.removeListener('exit', childHangup)
       childExited = true
       if (signal) {
         // If there is nothing else keeping the event loop alive,
@@ -78,3 +79,21 @@ module.exports = function (program, args, cb) {
 
   return child
 }
+
+function proxySignals (child) {
+  var listeners = {}
+  signalExit.signals().forEach(function (sig) {
+    listeners[sig] = function () {
+      child.kill(sig)
+    }
+    process.on(sig, listeners[sig])
+  })
+
+  return unproxySignals
+
+  function unproxySignals () {
+    for (var sig in listeners) {
+      process.removeListener(sig, listeners[sig])
+    }
+  }
+}
diff --git a/test/immortal-child.js b/test/immortal-child.js
new file mode 100644
index 0000000..d66e057
--- /dev/null
+++ b/test/immortal-child.js
@@ -0,0 +1,73 @@
+var node = process.execPath
+
+if (process.argv[2] === 'child')
+  child()
+else if (process.argv[2] === 'parent')
+  parent()
+else
+  test()
+
+function test () {
+  var t = require('tap')
+  var spawn = require('child_process').spawn
+  var proc = spawn(node, [__filename, 'parent'])
+
+  var out = ''
+  proc.stdout.on('data', function (c) { out += c })
+
+  var err = ''
+  proc.stderr.on('data', function (c) { err += c })
+
+  proc.on('exit', function (code, signal) {
+    clearTimeout(timer)
+  })
+  proc.on('close', function (code, signal) {
+    var actual = {
+      out: out,
+      err: err,
+      code: code,
+      signal: signal
+    }
+    var expect = {
+      out: /^(child alive\n)*child SIGINT received\n(child alive\n)*child exit null SIGTERM\n$/,
+      err: /^parent \d+\nchild \d+\n$/,
+      code: null,
+      signal: 'SIGTERM'
+    }
+    t.match(actual, expect)
+    t.end()
+  })
+
+  var time = 250
+  // coverage slows things down a bit
+  if (process.env._TAP_COVERAGE_)
+    time = 1000
+  var timer = setTimeout(function () {
+    proc.kill('SIGINT')
+    timer = setTimeout(function () {
+      proc.kill('SIGTERM')
+    }, time)
+  }, time)
+}
+
+function parent () {
+  console.error('parent', process.pid)
+  var fg = require('../')
+  fg(node, [ __filename, 'child' ])
+}
+
+function child () {
+  console.error('child', process.pid)
+  setInterval(function () {
+    console.log('child alive')
+  }, 200)
+  process.on('SIGINT', function () {
+    console.log('child SIGINT received')
+  })
+  process.on('SIGHUP', function () {
+    console.log('child SIGHUP received')
+  })
+  require('signal-exit')(function (code, signal) {
+    console.log('child exit', code, signal)
+  })
+}

-- 
Alioth's /usr/local/bin/git-commit-notice on /srv/git.debian.org/git/pkg-javascript/node-foreground-child.git