From ed91703f0870660a3159749878c273e3e61e64a3 Mon Sep 17 00:00:00 2001 From: Remy Sharp Date: Thu, 21 Nov 2019 11:14:45 +0000 Subject: [PATCH] fix: ubuntu loop waiting for sub processes Fixes #1633 Multiple parts but all localised to run.js: - Refactor the kill logic to be simpler - Consistently use pstree to determine sub process list - Pipe stderr through nodemon to squash `sh -c` error warning Bug was caused by waiting on multiple sub processes and if they all ended the logic would only subtract one from the count list (rather than the total number). I've refactored the code so that it doesn't use the `kill -0 ` as this was a little confusing to read (it's effectively a no op) and switched to using pstree to test if any sub processes are still running. The logic for killing the processes has also been refactored to simplify. Before it would fork the logic based on whether `ps` existed on the system. Now it uses the same logic with the exception of the kill signal sent - when `ps` isn't on the system, we have to send numeric signals (I can't remember how I found that out, but I do remember it was a painful process!). The last part required due to a side effect of the refactor on kill: when a kill signial is sent to `sh -c` the shell prints a warning. Details on how to replicate: https://git.io/Je6d0 To squash this, I track if the process is about to be killed (by flagging the sub process right before the kill function call) and if there's an error whilst shutdown is in effect, the error is only printed to nodemon's detailed output (using nodemon -V). --- LICENSE | 2 +- lib/monitor/run.js | 95 ++++++++++++++++++++++++---------------------- 2 files changed, 51 insertions(+), 46 deletions(-) diff --git a/LICENSE b/LICENSE index 31923ac4..19c91a2f 100644 --- a/LICENSE +++ b/LICENSE @@ -1,6 +1,6 @@ MIT License -Copyright (c) 2019 Remy Sharp, https://remysharp.com +Copyright (c) 2010 - present, Remy Sharp, https://remysharp.com Permission is hereby granted, free of charge, to any person obtaining a copy of this software and associated documentation files (the "Software"), to deal diff --git a/lib/monitor/run.js b/lib/monitor/run.js index bf48ab2d..4d9ca1ad 100644 --- a/lib/monitor/run.js +++ b/lib/monitor/run.js @@ -1,4 +1,4 @@ -var debug = require('debug')('nodemon'); +var debug = require('debug')('nodemon:run'); const statSync = require('fs').statSync; var utils = require('../utils'); var bus = utils.bus; @@ -10,34 +10,12 @@ var watch = require('./watch').watch; var config = require('../config'); var child = null; // the actual child process we spawn var killedAfterChange = false; -var noop = function () { }; +var noop = () => {}; var restart = null; var psTree = require('pstree.remy'); var path = require('path'); var signals = require('./signals'); -function waitForSubprocesses(subprocesses, callback) { - if (Array.isArray(subprocesses) && subprocesses.length > 0) { - // check if all old subprocesses have been terminated - exec('kill -0 ' + subprocesses.join(' '), (error) => { - const returnCode = error ? error.code : 0; - if (returnCode < 126) { // ignore command not found error - const stillRunning = subprocesses.length - returnCode; - if (stillRunning > 0) { - utils.log.status('still waiting for ' + stillRunning + - ' subprocess(es) to finish...'); - setTimeout(waitForSubprocesses.bind(this, subprocesses, callback), - 100); - return; - } - } - callback(); - }); - } else { - callback(); - } -} - function run(options) { var cmd = config.command.raw; @@ -55,11 +33,11 @@ function run(options) { var stdio = ['pipe', 'pipe', 'pipe']; if (config.options.stdout) { - stdio = ['pipe', process.stdout, process.stderr]; + stdio = ['pipe', process.stdout, 'pipe']; } if (config.options.stdin === false) { - stdio = [process.stdin, process.stdout, process.stderr]; + stdio = [process.stdin, process.stdout, 'pipe']; } var sh = 'sh'; @@ -161,6 +139,20 @@ function run(options) { bus.emit('message', message, sendHandle); }); } + } else { // else if not required… + + // this swallows a shell message that happens because we kill the sh -c + // more details here: https://git.io/Je6d0 + child.stderr.on('data', s => { + s = s.toString(); + + if (child.__nodemonRestart) { // this flag is set right before the kill + utils.log.detail('stderr: ' + s); + return; + } + + process.stderr.write(s); + }); } bus.emit('start'); @@ -284,6 +276,7 @@ function run(options) { /* Now kill the entire subtree of processes belonging to nodemon */ var oldPid = child.pid; if (child) { + child.__nodemonRestart = true; kill(child, config.signal, function () { // this seems to fix the 0.11.x issue with the "rs" restart command, // though I'm unsure why. it seems like more data is streamed in to @@ -340,6 +333,19 @@ function run(options) { } } +function waitForSubProcesses(pid, callback) { + debug('checking ps tree for pids of ' + pid); + psTree(pid, (err, pids) => { + if (!pids.length) { + return callback(); + } + + utils.log.status(`still waiting for ${pids.length} sub-process${ + pids.length > 2 ? 'es' : ''} to finish...`); + setTimeout(() => waitForSubProcesses(pid, callback), 1000); + }); +} + function kill(child, signal, callback) { if (!callback) { callback = function () { }; @@ -362,26 +368,25 @@ function kill(child, signal, callback) { // an array of PIDs that have spawned under nodemon, and we send each the // configured signal (default: SIGUSR2) signal, which fixes #335 // note that psTree also works if `ps` is missing by looking in /proc - const sig = signal.replace('SIG', ''); - psTree(child.pid, function (err, subprocesses) { - if (psTree.hasPS) { - spawn('kill', ['-s', sig].concat(subprocesses)) - .on('close', waitForSubprocesses.bind(this, subprocesses, - function () { - spawn('kill', ['-s', sig, child.pid]) - .on('close', callback); - })); - } else { - // make sure we kill from smallest to largest - const pids = subprocesses.slice().sort(); - pids.forEach(pid => { - exec('kill -' + signals[signal] + ' ' + pid, () => { }); - }); - waitForSubprocesses(subprocesses, function () { - exec('kill -' + signals[signal] + ' ' + child.pid, () => { }); - callback(); - }) + let sig = signal.replace('SIG', ''); + + psTree(child.pid, function (err, pids) { + // if ps isn't native to the OS, then we need to send the numeric value + // for the signal during the kill, `signals` is a lookup table for that. + if (!psTree.hasPS) { + sig = signals[signal]; } + + // the sub processes need to be killed from smallest to largest + debug('sending kill signal to ' + pids.join(', ')); + + pids.sort().forEach(pid => exec(`kill -${sig} ${pid}`, noop)); + + waitForSubProcesses(child.pid, () => { + // finally kill the main user process + exec(`kill -${sig} ${child.pid}`, callback); + }); + }); }