Skip to content

Commit

Permalink
fix(server): fix uncaught exceptions triggering route lookups (#1717)
Browse files Browse the repository at this point in the history
  • Loading branch information
mridgway authored and hekike committed Nov 16, 2018
1 parent 11d6458 commit e49cb3b
Show file tree
Hide file tree
Showing 2 changed files with 103 additions and 7 deletions.
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 (normalizedErrName && self.listeners(normalizedErrName).length > 0) {
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 {
} 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
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

0 comments on commit e49cb3b

Please sign in to comment.