Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better Master Process Exit Handling #4

Merged
merged 3 commits into from Apr 17, 2015
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 7 additions & 1 deletion README.md
Expand Up @@ -62,7 +62,12 @@ var manager = new ClusterManager({

// Tell it not to kill the master process on an un-handled error
// (sometimes useful, not recommended)
killOnError: false
killOnError: false,

// Perform some action before the master process exits due to an error
beforeExit: function() {
// ...
}
});

// Start the cluster!
Expand All @@ -84,6 +89,7 @@ To aid such specialized behaviors the `ClusterManager` class was designed to be
extendable via prototypal inheritance. Furthermore, instances expose the node
`cluster` directly so additional eventing can easily be added.


**Example: Adding additional cluster event listeners**
```js
var app = require('./lib/app.js');
Expand Down
27 changes: 25 additions & 2 deletions index.js
Expand Up @@ -64,6 +64,8 @@ module.exports = ClusterManager;
* to 'cluster-man'.
* @param {Boolean} opt.killOnError=true Whether or not to kill the master
* process on and unhandled error.
* @param {function} beforeExit Callback to execute before the master process
* exits in response to an error.
* @throws Error If a opt.worker was not specified or was not a function.
*/
function ClusterManager(opts) {
Expand All @@ -78,7 +80,8 @@ function ClusterManager(opts) {
debugScope: process.env.CLUSTER_DEBUG || 'cluster-man',
master: noop,
numWorkers: process.env.CLUSTER_WORKERS || os.cpus().length,
killOnError: true
killOnError: true,
beforeExit: noop
});

this._addLogger('info', [this.options.debugScope, 'info'].join(':'));
Expand All @@ -93,6 +96,11 @@ function ClusterManager(opts) {
this.log.warning('Number of workers not specified, using default.');
}

if (!isFunction(this.options.beforeExit)) {
this.log.warning('Before exit callback is not a function, removing.');
this.options.beforeExit = noop;
}

this.workers = [];

// This is here to expose the cluster without having to re-require in the
Expand Down Expand Up @@ -160,6 +168,15 @@ ClusterManager.prototype._startMaster = function() {
});
};

/**
* Runs before exit callback and exits the master process.
* @param {Error} [err] Error that caused the master process to exit.
*/
ClusterManager.prototype._exitMaster = function (err) {
this.options.beforeExit(err);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this take a cb? otherwise we might exit if the beforeExit function is async

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good point, on it!

process.exit(err ? 1 : 0);
};

/**
* Starts a cluster worker. Simply executes the provided worker callback.
*/
Expand Down Expand Up @@ -230,6 +247,12 @@ ClusterManager.prototype.exit = function (worker, code, signal) {
self.workers.splice(i, 1);
}
});

// If all the workers have been killed, exit the process
if (this.workers.length === 0) {
this.log.error('Cluster fatal: all worker have died. Master process exiting.');
this._exitMaster(new Error('All workers have died.'));
}
};

/**
Expand Down Expand Up @@ -260,6 +283,6 @@ ClusterManager.prototype.masterError = function(err) {
this.log.error('Unhandled master error: ' + err.stack);
if (this.options.killOnError) {
this.log.error('Cluster fatal: unhandled error in master process, exiting.');
process.exit(1);
this._exitMaster(err);
}
};
2 changes: 1 addition & 1 deletion package.json
@@ -1,6 +1,6 @@
{
"name": "cluster-man",
"version": "0.0.1",
"version": "1.0.0",
"description": "Extendable and easy-to-use node cluster management.",
"main": "index.js",
"scripts": {
Expand Down
35 changes: 28 additions & 7 deletions test/events.js
Expand Up @@ -124,6 +124,29 @@ describe('cluster-man', function () {
});
done();
});

it('should exit the master process if all workers exit', function (done) {
var stub = sinon.stub(manager, '_exitMaster');
var log = sinon.spy(manager.log, 'error');

// Need to indirectly iterate over the workers, since `manager.workers`
// is modified by the 'exit' event handler
manager.workers.map(function (worker) {
return worker;
}).forEach(function (worker) {
manager.cluster.emit('exit', worker, 1, 'SIGINT');
});

expect(stub.calledOnce)
.to.be.true();
expect(stub.calledWithMatch({ message: 'All workers have died.' }))
.to.be.true();
expect(log.calledWithMatch('Cluster fatal'))
.to.be.true();

manager._exitMaster.restore();
done();
});
}); // end 'exit'

describe('online', function () {
Expand Down Expand Up @@ -205,10 +228,9 @@ describe('cluster-man', function () {
manager._startMaster();
});

it('should exit the master process on unlocked errors', function (done) {
sinon.stub(process, 'exit', function (code) {
expect(code).to.equal(1);
process.exit.restore();
it('should exit the master process on uncaught errors', function (done) {
var stub = sinon.stub(manager, '_exitMaster', function (err) {
expect(err).to.equal(errorObject);
done();
});
manager._startMaster();
Expand All @@ -219,10 +241,9 @@ describe('cluster-man', function () {
worker: noop,
killOnError: false
});
sinon.stub(process, 'exit');
var stub = sinon.stub(manager, '_exitMaster');
manager.masterError(new Error('Error'));
expect(process.exit.callCount).to.equal(0);
process.exit.restore();
expect(stub.callCount).to.equal(0);
done();
});
}); // end 'masterError'
Expand Down
52 changes: 52 additions & 0 deletions test/methods.js
Expand Up @@ -135,6 +135,15 @@ describe('cluster-man', function () {
expect(manager.options.killOnError).to.be.false();
done();
});

it('should not allow non-function beforeExit option', function(done) {
var manager = new ClusterManager({
worker: noop,
beforeExit: 'not a function'
});
expect(manager.options.beforeExit).to.equal(noop);
done();
});
}); // end 'constructor'

describe('_addLogger', function () {
Expand Down Expand Up @@ -228,6 +237,49 @@ describe('cluster-man', function () {
});
}); // end '_startMaster'

describe('_exitMaster', function() {
beforeEach(function (done) {
sinon.stub(process, 'exit');
done();
});

afterEach(function (done) {
process.exit.restore();
done();
});

it('should execute the `beforeExit` callback', function(done) {
var manager = new ClusterManager({
worker: noop,
beforeExit: function() {
return 'beforeExit';
}
});
var spy = sinon.spy(manager.options, 'beforeExit');
var err = new Error('Error');
manager._exitMaster(err);
expect(spy.calledOnce).to.be.true();
expect(spy.calledWith(err)).to.be.true();
done();
});

it('should exit the process with code 1 when given an error', function(done) {
var manager = new ClusterManager(noop);
manager._exitMaster(new Error('error'));
expect(process.exit.calledOnce).to.be.true();
expect(process.exit.calledWith(1)).to.be.true();
done();
});

it('should exit the process with code 0 when not given an error', function (done) {
var manager = new ClusterManager(noop);
manager._exitMaster();
expect(process.exit.calledOnce).to.be.true();
expect(process.exit.calledWith(0)).to.be.true();
done();
});
}); // end '_exitMaster'

describe('_startWorker', function () {
it('should call the worker callback', function (done) {
var manager = new ClusterManager(noop);
Expand Down