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

[Restify 7] Fix uncaught exceptions triggering route lookups #1717

Merged
merged 3 commits into from Nov 16, 2018
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
20 changes: 13 additions & 7 deletions lib/server.js
Expand Up @@ -1077,7 +1077,7 @@ Server.prototype._onHandlerError = function _onHandlerError(
var self = this;

// Call route by name
if (_.isString(err)) {
if (!isUncaught && _.isString(err)) {
var routeName = err;
var routeHandler = self.router.lookupByName(routeName, req, res);

Expand Down Expand Up @@ -1322,22 +1322,26 @@ Server.prototype._emitErrorEvents = function _emitErrorEvents(
cb
) {
var self = this;
var errName = errEvtNameFromError(err);
// Error can be of any type: undefined, Error, Number, etc. so we need
// to protect ourselves from trying to resolve names from non Error objects
var errName = err && err.name;
var normalizedErrName = errName && errEvtNameFromError(err);

req.log.trace(
{
err: err,
errName: errName
errName: normalizedErrName
},
'entering emitErrorEvents',
err.name
errName
);

var errEvtNames = [];

// if we have listeners for the specific error, fire those first.
if (self.listeners(errName).length > 0) {
errEvtNames.push(errName);
// if there's no error name, we should not emit an event
if (errName && self.listeners(normalizedErrName).length > 0) {
hekike marked this conversation as resolved.
Show resolved Hide resolved
errEvtNames.push(normalizedErrName);
}

// or if we have a generic error listener. always fire generic error event
Expand Down Expand Up @@ -1482,9 +1486,11 @@ function errEvtNameFromError(err) {
} else if (err.name === 'InvalidVersionError') {
// remap the name for router errors
return 'VersionNotAllowed';
} else {
hekike marked this conversation as resolved.
Show resolved Hide resolved
} else if (err.name) {
return err.name.replace(/Error$/, '');
}
// If the err is not an Error, then just return an empty string
return '';
}

/**
Expand Down
90 changes: 90 additions & 0 deletions test/server.test.js
Expand Up @@ -2477,6 +2477,96 @@ test('should emit error with multiple next calls with strictNext', function(t) {
});
});

test('uncaughtException should not trigger named routeHandler', function(t) {
SERVER.get(
{
name: 'foo',
path: '/foo'
},
function(req, res, next) {
throw 'bar'; //eslint-disable-line no-throw-literal
}
);

SERVER.get(
{
name: 'bar',
path: '/bar'
},
function(req, res, next) {
// This code should not run, but we can test against the status code
hekike marked this conversation as resolved.
Show resolved Hide resolved
res.send(200);
next();
}
);

CLIENT.get('/foo', function(err, _, res) {
t.ok(err);
t.equal(res.statusCode, 500);
t.end();
});
});

test('uncaughtException should handle thrown undefined literal', function(t) {
SERVER.get(
{
name: 'foo',
path: '/foo'
},
function(req, res, next) {
throw undefined; //eslint-disable-line no-throw-literal
}
);

SERVER.get(
{
name: 'bar',
path: '/bar'
},
function(req, res, next) {
// This code should not run, but we can test against the status code
res.send(200);
next();
}
);

CLIENT.get('/foo', function(err, _, res) {
t.ok(err);
t.equal(res.statusCode, 500);
t.end();
});
});

test('uncaughtException should handle thrown Number', function(t) {
SERVER.get(
{
name: 'foo',
path: '/foo'
},
function(req, res, next) {
throw 1; //eslint-disable-line no-throw-literal
}
);

SERVER.get(
{
name: 'bar',
path: '/bar'
},
function(req, res, next) {
// This code should not run, but we can test against the status code
res.send(200);
next();
}
);

CLIENT.get('/foo', function(err, _, res) {
t.ok(err);
t.equal(res.statusCode, 500);
t.end();
});
});

test('should have proxy event handlers as instance', function(t) {
var server = restify.createServer({
handleUpgrades: false
Expand Down