From abc27cd5cad41d3cdfb199f0ada6e54e387b9b25 Mon Sep 17 00:00:00 2001 From: Remy Sharp Date: Tue, 10 Dec 2013 00:06:25 +0000 Subject: [PATCH] Correct signal passing. Fixes #232. Also more linting, tweaks to FAQ & contributing and added .jshint file. Also adds test for signal test (though needs mocha to timeout at 15 seconds!) --- .jshintrc | 14 +++++++++ CONTRIBUTING.md | 4 +-- faq.md | 9 ++++++ lib/monitor/run.js | 48 ++++++++++++++++++++++++++---- test/fixtures/sigint.js | 7 +++++ test/{monitor => fork}/run.test.js | 0 test/misc/sigint.test.js | 47 +++++++++++++++++++++++++++++ test/monitor/change-detect.test.js | 2 +- 8 files changed, 122 insertions(+), 9 deletions(-) create mode 100644 .jshintrc create mode 100644 test/fixtures/sigint.js rename test/{monitor => fork}/run.test.js (100%) create mode 100644 test/misc/sigint.test.js diff --git a/.jshintrc b/.jshintrc new file mode 100644 index 0000000..571b952 --- /dev/null +++ b/.jshintrc @@ -0,0 +1,14 @@ +{ + "browser": true, + "camelcase": true, + "curly": true, + "devel": true, + "eqeqeq": true, + "forin": true, + "indent": 2, + "noarg": true, + "node": true, + "quotmark": "single", + "undef": true, + "unused": true +} \ No newline at end of file diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 2f3ea62..e32e37b 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -1,8 +1,8 @@ # Contribute -WIP - but here's the hints +WIP - but here's the TL;DR - .jshintrc - new command line options are generally discouraged - ensure style is consistent -- add tests for newly added code \ No newline at end of file +- add tests for newly added code (and try to mirror directory and file structure if possible) \ No newline at end of file diff --git a/faq.md b/faq.md index f84ce70..fdd8bb5 100644 --- a/faq.md +++ b/faq.md @@ -48,3 +48,12 @@ To test this, you can kill the server.js process and forever will restart it. If Note that I *would not* recommend using nodemon in a production environment - but that's because I wouldn't want it restart without my explicit instruction. +## nodemon tries to run two scripts + +If you see nodemon trying to run two scripts, like: + +``` +9 Dec 23:52:58 - [nodemon] starting `node ./app.js fixtures/sigint.js` +``` + +This is because the main script argument (`fixtures/sigint.js` in this case) wasn't found, and a `package.json`'s main file *was* found. ie. to solve, double check the path to your script is correct. \ No newline at end of file diff --git a/lib/monitor/run.js b/lib/monitor/run.js index afe03d3..a9b7915 100644 --- a/lib/monitor/run.js +++ b/lib/monitor/run.js @@ -1,3 +1,4 @@ +'use strict'; var utils = require('../utils'), bus = utils.bus, childProcess = require('child_process'), @@ -8,6 +9,7 @@ var utils = require('../utils'), child = null, // the actual child process we spawn killedAfterChange = false, timeout = 1000, // check every 1 second + noop = function() {}, restart = null; function run(options) { @@ -16,6 +18,7 @@ function run(options) { utils.log.status('starting `' + command.executable + (command.args.length ? ' ' + command.args.join(' ') : '') + '`'); + /*jshint validthis:true*/ restart = run.bind(this, options); run.restart = restart; @@ -41,7 +44,7 @@ function run(options) { child.on('error', function (error) { if (error.code === 'ENOENT') { - utils.log.error('unable to spawn executable: ' + executable); + utils.log.error('unable to spawn executable: ' + command.executable); process.exit(1); } }); @@ -74,7 +77,7 @@ function run(options) { }); process.on('exit', function () { - if (child) bus.emit('kill'); + if (child) { bus.emit('kill'); } }); run.kill = function () { @@ -135,17 +138,17 @@ run.command = function (options) { return { executable: executable, args: args - } + }; }; // stubbed out for now, filled in during run -run.kill = function () {}; -run.restart = function () {}; +run.kill = noop; +run.restart = noop; bus.on('quit', function () { // remove event listener var exit = function () { - exit = function () {}; // null out in case of race condition + exit = noop; // null out in case of race condition process.exit(0); }; @@ -161,5 +164,38 @@ bus.on('restart', function () { run.kill(); }); +// remove the flag file on exit +process.on('exit', function (code) { + utils.log.detail('exiting'); + child.kill(); +}); + +// because windows borks when listening for the SIG* events +if (!utils.isWindows) { + // usual suspect: ctrl+c exit + process.on('SIGINT', function () { + var exit = function () { + exit = noop; + child.kill('SIGINT'); + process.exit(0); + }; + + if (child) { + child.removeAllListeners('exit'); + child.on('exit', exit); + child.kill('SIGINT'); + // give up waiting for the kids after 10 seconds + setTimeout(exit, 10 * 1000); + } else { + exit(); + } + }); + + process.on('SIGTERM', function () { + child.kill('SIGTERM'); + process.exit(0); + }); +} + module.exports = run; \ No newline at end of file diff --git a/test/fixtures/sigint.js b/test/fixtures/sigint.js new file mode 100644 index 0000000..fe98422 --- /dev/null +++ b/test/fixtures/sigint.js @@ -0,0 +1,7 @@ +process.on('SIGINT', function() { + // do nothing here +}); +// timer, to keep process running +setInterval(function() { + // console.log('working'); +}, 1000); diff --git a/test/monitor/run.test.js b/test/fork/run.test.js similarity index 100% rename from test/monitor/run.test.js rename to test/fork/run.test.js diff --git a/test/misc/sigint.test.js b/test/misc/sigint.test.js new file mode 100644 index 0000000..b072835 --- /dev/null +++ b/test/misc/sigint.test.js @@ -0,0 +1,47 @@ +'use strict'; +/*global describe:true, it: true */ +var utils = require('../utils'), + assert = require('assert'), + path = require('path'), + appjs = path.relative(process.cwd(), path.resolve(__dirname, '..', 'fixtures', 'sigint.js')), + match = utils.match, + cleanup = utils.cleanup, + run = utils.run; + +describe('terminal signals', function () { + it('should kill child with SIGINT (after ~10 seconds)', function (done) { + var childPID = null; + + var p = run(appjs, { + output: function (data) { + if (match(data, 'pid: ')) { + data.replace(/pid: (\d+)/, function (m) { + childPID = m; + }); + } + }, + error: function (data) { + assert(false, 'nodemon failed with ' + data); + cleanup(p, done); + } + }); + + p.on('message', function (event) { + if (event.type === 'start') { + setTimeout(function () { + p.kill('SIGINT'); + }, 1000); + } + }).on('exit', function () { + // check if the child process is still running + try { + process.kill(childPID, 0); + assert(false, 'child is still running at ' + childPID); + } catch (e) { + assert(true, 'child process was not running'); + } + done(); + }); + }); + +}); diff --git a/test/monitor/change-detect.test.js b/test/monitor/change-detect.test.js index 617165f..1f6a2c7 100644 --- a/test/monitor/change-detect.test.js +++ b/test/monitor/change-detect.test.js @@ -20,7 +20,7 @@ describe('nodemon simply running', function () { }, error: function (data) { assert(false, 'nodemon failed with ' + data); - cleanup(p, done, new Error(data)); + cleanup(p, done); } }); });