Skip to content

Commit

Permalink
fix: pipe stderr correctly
Browse files Browse the repository at this point in the history
Fixes #1638

This also simplifies the shutdown process by sending the kill signal to
the sub-process - thus ensuring we don't get the weird "User defined
singal" from the shell.

This commit also includes a test to ensure this doesn't happen again.
  • Loading branch information
remy committed Dec 4, 2019
1 parent ed91703 commit 47dfb8b
Show file tree
Hide file tree
Showing 2 changed files with 37 additions and 22 deletions.
25 changes: 7 additions & 18 deletions lib/monitor/run.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,11 +33,11 @@ function run(options) {
var stdio = ['pipe', 'pipe', 'pipe'];

if (config.options.stdout) {
stdio = ['pipe', process.stdout, 'pipe'];
stdio = ['pipe', process.stdout, process.stderr];
}

if (config.options.stdin === false) {
stdio = [process.stdin, process.stdout, 'pipe'];
stdio = [process.stdin, process.stdout, process.stderr];
}

var sh = 'sh';
Expand Down Expand Up @@ -99,6 +99,8 @@ function run(options) {
if (shouldFork) {
var forkArgs = cmd.args.slice(1);
var env = utils.merge(options.execOptions.env, process.env);
stdio.pop();
stdio.push(process.stderr);
stdio.push('ipc');
child = fork(options.execOptions.script, forkArgs, {
env: env,
Expand All @@ -110,7 +112,7 @@ function run(options) {
} else {
utils.log.detail('spawning');
child = spawn.apply(null, spawnArgs);
debug('spawn', sh, shFlag, args)
debug('spawn', sh, shFlag, args);
}

if (config.required) {
Expand Down Expand Up @@ -139,20 +141,6 @@ 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');
Expand Down Expand Up @@ -276,7 +264,6 @@ 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
Expand Down Expand Up @@ -380,6 +367,8 @@ function kill(child, signal, callback) {
// the sub processes need to be killed from smallest to largest
debug('sending kill signal to ' + pids.join(', '));

child.kill(signal);

pids.sort().forEach(pid => exec(`kill -${sig} ${pid}`, noop));

waitForSubProcesses(child.pid, () => {
Expand Down
34 changes: 30 additions & 4 deletions test/fork/run.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,19 +5,45 @@ var assert = require('assert'),
run = utils.run;

describe('nodemon fork', function () {
it('should start a fork', function (done) {
var p = run(appjs, {
it('should not show user-signal', done => {
var p = run({ exec: 'bin/nodemon.js',
args: [ '-V', '-x', 'echo running && sleep 20' ] }, {
error: function (data) {
p.send('quit');
if (data.trim().indexOf('signal') !== -1) {
return done(new Error('Signal incorrectly shown'));
}

done(new Error(data));
},
output: function (data) {
if (data.trim().indexOf('signal') !== -1) {
done(new Error('Signal incorrectly shown'));
}
}
});

let started = false;
p.on('message', function (event) {
if (event.type === 'start') {
if (!started) {
p.send('restart');
started = true;
} else {
p.send('quit');
assert(true, 'nodemon started');
done();
}

}
});
});

it('should start a fork', function (done) {
var p = run(appjs, {
error: function (data) {
p.send('quit');
assert(true, 'nodemon started');
done();
done(new Error(data));
}
});
});
Expand Down

0 comments on commit 47dfb8b

Please sign in to comment.