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.

Sign up for GitHub

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

Jump to bottom

Fix invalid triggers of the uncaughtException handler #1710

Merged
merged 3 commits into from Oct 30, 2018
Merged

Fix invalid triggers of the uncaughtException handler #1710

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
99b9238
Fix invalid triggers of the uncaughtException handler
mridgway Oct 26, 2018
83c6853
Remove duplicate check for handleUncaughtExceptions
mridgway Oct 29, 2018
4bfceb0
Add clarification to internal errors test
mridgway Oct 29, 2018
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
3 changes: 2 additions & 1 deletion lib/index.js
View file
Open in desktop
Expand Up @@ -89,7 +89,8 @@ function createServer(options) {
e
) {
if (
this.listeners('uncaughtException').length > 1 ||
(opts.handleUncaughtExceptions &&
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this already gated (opts.handleUncaughtExceptions) on L84?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it is indeed. I'll remove this check.

this.listeners('uncaughtException').length > 1) ||
res.headersSent
) {
return false;
Expand Down
22 changes: 17 additions & 5 deletions lib/server.js
View file
Open in desktop
Expand Up @@ -816,7 +816,7 @@ Server.prototype._onRequest = function _onRequest(req, res) {
handlerDomain.add(req);
handlerDomain.add(res);
handlerDomain.on('error', function onError(err) {
self._onHandlerError(err, req, res);
self._onHandlerError(err, req, res, true);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this would still make errors emitted on objects bound to the domain (in this case req and res) trigger an uncaughtException event. It seems that would also be a problem?

If we consider that is indeed not what the behavior of handleUncaughtExceptions should be, we should set the fourth parameter of _onHandlerError to true only if err.domainThrown === true.

@mridgway @DonutEspresso Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe that behavior is consistent with Restify versions <7. I'm not sure what the original intention was though.

I agree that it would make more sense to filter to only uncaught exceptions.

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe that behavior is consistent with Restify versions <7.

I just verified that looking at the 6.x branch of this repository and this is correct. Thus I'm fine with keeping that behavior, at least for now.

});
handlerDomain.run(function run() {
self._runPre(req, res);
Expand Down Expand Up @@ -1065,9 +1065,15 @@ Server.prototype._onHandlerStop = function _onHandlerStop(req, res) {
* @param {Error|String|undefined} err - router handler error or route name
* @param {Request} req - request
* @param {Response} res - response
* @param {boolean} isUncaught - whether the error is uncaught
* @returns {undefined} no return value
*/
Server.prototype._onHandlerError = function _onHandlerError(err, req, res) {
Server.prototype._onHandlerError = function _onHandlerError(
err,
req,
res,
isUncaught
) {
var self = this;

// Call route by name
Expand Down Expand Up @@ -1098,7 +1104,7 @@ Server.prototype._onHandlerError = function _onHandlerError(err, req, res) {
res.err = res.err || err;

// Error happened in router handlers
self._routeErrorResponse(req, res, err);
self._routeErrorResponse(req, res, err, isUncaught);
};

/**
Expand Down Expand Up @@ -1256,16 +1262,22 @@ Server.prototype._finishReqResCycle = function _finishReqResCycle(
* @param {Request} req - the request object
* @param {Response} res - the response object
* @param {Error} err - error
* @param {boolean} isUncaught - whether the error is uncaught
* @returns {undefined} no return value
*/
Server.prototype._routeErrorResponse = function _routeErrorResponse(
req,
res,
err
err,
isUncaught
) {
var self = this;

if (self.listenerCount('uncaughtException') > 1) {
if (
isUncaught &&
self.handleUncaughtExceptions &&
self.listenerCount('uncaughtException') > 1
) {
self.emit('uncaughtException', req, res, req.route, err);
return;
}
Expand Down
36 changes: 36 additions & 0 deletions test/server.test.js
View file
Open in desktop
Expand Up @@ -674,6 +674,42 @@ test('GH-77 uncaughtException (default behavior)', function(t) {
});
});

// eslint-disable-next-line
test('handleUncaughtExceptions should not call handler for internal errors', function(t) {
SERVER.get('/', function(req, res, next) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm a little bit confused about this test setup. I think we trying to verify restify/router errors (e.g., 404) don't trigger uncaught exceptions? If yes, should we just remove the get route and have the client do a get against a non-existent route?

Copy link
Contributor Author

@mridgway mridgway Oct 29, 2018 •
edited

Choose a reason for hiding this comment

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

There seems to be a behavior difference if you have no routes (which 404s) and having only GET routes and trying HEAD (results in MethodNotAllowed error). I agree that this makes the test confusing, but I think if the behavior difference should be fixed, it should be fixed separately.

Copy link
Member

Choose a reason for hiding this comment

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

Gotcha - thanks for context! I expect this is related to CORS use cases - do you mind adding all of this as a comment to the test? Otherwise, it's a bit confusing to register the empty / that we actually don't expect to be run or triggered. Maybe we could do an assert.fail('should not run') or something inside the GET handler for clarity as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. I added a comment and the assertion that you mentioned.

res.send('Hello');
});

SERVER.on('uncaughtException', function throwError(err) {
t.ifError(err);
t.end();
});

CLIENT.head('/', function(err, _, res) {
t.ok(err);
t.equal(res.statusCode, 405);
t.end();
});
});

// eslint-disable-next-line
test('handleUncaughtExceptions should not call handler for next(new Error())', function(t) {
Copy link
Member

Choose a reason for hiding this comment

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

👍

SERVER.get('/', function(req, res, next) {
next(new Error('I am not fatal'));
});

SERVER.on('uncaughtException', function throwError(err) {
t.ifError(err);
t.end();
});

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

test('GH-77 uncaughtException (with custom handler)', function(t) {
SERVER.on('uncaughtException', function(req, res, route, err) {
res.send(204);
Expand Down