From 60a133f36c0145d83083690e435f54a1ad0cff95 Mon Sep 17 00:00:00 2001 From: Chris Kleeschulte Date: Fri, 11 Sep 2015 12:39:33 -0400 Subject: [PATCH 1/3] Multiple SIGINT signals sent to node process while in shutdown cycle. - Overrode the sigint listener if in the shutdown cycle. This prevents control being returned to the pty before the node process is fully halted. --- lib/scaffold/start.js | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/scaffold/start.js b/lib/scaffold/start.js index e4528324..0bedf0ab 100644 --- a/lib/scaffold/start.js +++ b/lib/scaffold/start.js @@ -153,6 +153,7 @@ function registerExitHandlers(_process, node) { }); } if (options.sigint) { + _process.on('SIGINT', function(){}); cleanShutdown(_process, node); } } From 51355c51f1dd329c66152702b7ef780dac951b10 Mon Sep 17 00:00:00 2001 From: Chris Kleeschulte Date: Fri, 11 Sep 2015 16:26:23 -0400 Subject: [PATCH 2/3] Multiple sigints sent to the node process - Added a simple replacement function after the first sigint is handled. - Added unit tests for the exitHandler, requiring it to be exposed from the module. --- lib/scaffold/start.js | 53 ++++++++++++++++------------- test/scaffold/start.unit.js | 66 +++++++++++++++++++++++++++++++++++++ 2 files changed, 97 insertions(+), 22 deletions(-) diff --git a/lib/scaffold/start.js b/lib/scaffold/start.js index 0bedf0ab..512733ad 100644 --- a/lib/scaffold/start.js +++ b/lib/scaffold/start.js @@ -138,31 +138,39 @@ function cleanShutdown(_process, node) { * @param {Node} node */ function registerExitHandlers(_process, node) { - - function exitHandler(options, err) { - if (err) { - log.error('uncaught exception:', err); - if(err.stack) { - log.error(err.stack); - } - node.stop(function(err) { - if(err) { - log.error('Failed to stop services: ' + err); - } - _process.exit(-1); - }); - } - if (options.sigint) { - _process.on('SIGINT', function(){}); - cleanShutdown(_process, node); - } - } - //catches uncaught exceptions - _process.on('uncaughtException', exitHandler.bind(null, {exit:true})); + _process.on('uncaughtException', exitHandler.bind(null, {exit:true}, _process, node)); //catches ctrl+c event - _process.on('SIGINT', exitHandler.bind(null, {sigint:true})); + _process.on('SIGINT', exitHandler.bind(null, {sigint:true}, _process, node)); +} + +/** + * Will handle all the shutdown tasks that need to take place to ensure a safe exit + * @param {Object} options + * @param {String} options.sigint - The signal given was a SIGINT + * @param {Array} options.exit - The signal given was an uncaughtException + * @param {Object} _process - The Node.js process + * @param {Node} node + * @param {Error} error +*/ +function exitHandler(options, _process, node, err) { + if (err) { + log.error('uncaught exception:', err); + if(err.stack) { + log.error(err.stack); + } + node.stop(function(err) { + if(err) { + log.error('Failed to stop services: ' + err); + } + _process.exit(-1); + }); + } + if (options.sigint) { + _process.on('SIGINT', function(){}); + start.cleanShutdown(_process, node); + } } /** @@ -255,6 +263,7 @@ function spawnChildProcess(datadir, _process) { module.exports = start; module.exports.registerExitHandlers = registerExitHandlers; +module.exports.exitHandler = exitHandler; module.exports.registerSyncHandlers = registerSyncHandlers; module.exports.setupServices = setupServices; module.exports.spawnChildProcess = spawnChildProcess; diff --git a/test/scaffold/start.unit.js b/test/scaffold/start.unit.js index 3cd8c9de..b3aafc6d 100644 --- a/test/scaffold/start.unit.js +++ b/test/scaffold/start.unit.js @@ -304,12 +304,78 @@ describe('#start', function() { }); it('call spawnChildProcess if there is a config option to do so', function() { start(options); + registerSync.callCount.should.equal(1); + registerExit.callCount.should.equal(1); spawn.callCount.should.equal(1); }); it('not call spawnChildProcess if there is not an option to do so', function() { options.config.daemon = false; start(options); + registerSync.callCount.should.equal(1); + registerExit.callCount.should.equal(1); spawn.callCount.should.equal(0); }); }); + describe('#registerExitHandlers', function() { + var stub; + var registerExitHandlers = require('../../lib/scaffold/start').registerExitHandlers; + + before(function() { + stub = sinon.stub(process, 'on'); + }); + + after(function() { + stub.restore(); + }); + + it('should setup two listeners on process when registering exit handlers', function() { + registerExitHandlers(process, {}); + stub.callCount.should.equal(2); + }); + + describe('#exitHandler', function() { + var sandbox; + var cleanShutdown; + var exitHandler; + var logStub; + + before(function() { + sandbox = sinon.sandbox.create(); + var start = require('../../lib/scaffold/start'); + var log = require('../../lib').log; + logStub = sandbox.stub(log, 'error'); + cleanShutdown = sandbox.stub(start, 'cleanShutdown', function() {}); + exitHandler = require('../../lib/scaffold/start').exitHandler; + }); + + after(function() { + sandbox.restore(); + }); + + it('should replace the listener for SIGINT after the first SIGINT is handled', function() { + var options = { sigint: true }; + var node = {}; + var replacementFunc = sinon.stub(); + replacementFunc.withArgs('SIGINT', function(){}); + var testProcess = { + on: replacementFunc + } + exitHandler(options, testProcess, node); + cleanShutdown.callCount.should.equal(1); + replacementFunc.callCount.should.equal(1); + }); + + it('should log all errors and stops the services nonetheless', function() { + var options = { sigint: true }; + var stop = sinon.stub(); + var node = { + stop: stop + }; + exitHandler(options, process, node, new Error('some error')); + logStub.callCount.should.equal(2); + stop.callCount.should.equal(1); + }); + + }); + }); }); From 7b50f5ff3d1448a60a46bd590ac40aadf8b41367 Mon Sep 17 00:00:00 2001 From: Chris Kleeschulte Date: Mon, 14 Sep 2015 13:15:55 -0400 Subject: [PATCH 3/3] Switched to using a flag instead of using a replacement. --- lib/scaffold/start.js | 7 +++++-- test/scaffold/start.unit.js | 10 +++------- 2 files changed, 8 insertions(+), 9 deletions(-) diff --git a/lib/scaffold/start.js b/lib/scaffold/start.js index 512733ad..0d24228e 100644 --- a/lib/scaffold/start.js +++ b/lib/scaffold/start.js @@ -9,6 +9,7 @@ var $ = bitcore.util.preconditions; var log = index.log; var child_process = require('child_process'); var fs = require('fs'); +var shuttingDown = false; log.debug = function() {}; @@ -168,8 +169,10 @@ function exitHandler(options, _process, node, err) { }); } if (options.sigint) { - _process.on('SIGINT', function(){}); - start.cleanShutdown(_process, node); + if (!shuttingDown) { + shuttingDown = true; + start.cleanShutdown(_process, node); + } } } diff --git a/test/scaffold/start.unit.js b/test/scaffold/start.unit.js index b3aafc6d..c955cbf9 100644 --- a/test/scaffold/start.unit.js +++ b/test/scaffold/start.unit.js @@ -355,14 +355,10 @@ describe('#start', function() { it('should replace the listener for SIGINT after the first SIGINT is handled', function() { var options = { sigint: true }; var node = {}; - var replacementFunc = sinon.stub(); - replacementFunc.withArgs('SIGINT', function(){}); - var testProcess = { - on: replacementFunc - } - exitHandler(options, testProcess, node); + exitHandler(options, process, node); + cleanShutdown.callCount.should.equal(1); + exitHandler(options, process, node); cleanShutdown.callCount.should.equal(1); - replacementFunc.callCount.should.equal(1); }); it('should log all errors and stops the services nonetheless', function() {