Fix invalid triggers of the uncaughtException handler #1710
Conversation
The previous (prior to 7.x) functionality for uncaught exceptions only triggered the `uncaughtException` handler if it was truly an uncaught thrown error. Right now in Restify 7, if you turn on `handleUncaughtExceptions` and add a listener for the event, all errors from `next(err)` and internal errors like `MethodNotAllowed` will trigger the `uncaughtException` handler.
lib/index.js
Outdated
| ) { | ||
| if ( | ||
| this.listeners('uncaughtException').length > 1 || | ||
| (opts.handleUncaughtExceptions && |
There was a problem hiding this comment.
Isn't this already gated (opts.handleUncaughtExceptions) on L84?
There was a problem hiding this comment.
Yeah, it is indeed. I'll remove this check.
|
|
||
| // eslint-disable-next-line | ||
| test('handleUncaughtExceptions should not call handler for internal errors', function(t) { | ||
| SERVER.get('/', function(req, res, next) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Makes sense. I added a comment and the assertion that you mentioned.
| }); | ||
|
|
||
| // eslint-disable-next-line | ||
| test('handleUncaughtExceptions should not call handler for next(new Error())', function(t) { |
| handlerDomain.add(res); | ||
| handlerDomain.on('error', function onError(err) { | ||
| self._onHandlerError(err, req, res); | ||
| self._onHandlerError(err, req, res, true); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
3 participants
Pre-Submission Checklist
make prepushIssues
Closes:
Changes
The previous (prior to 7.x) functionality for uncaught exceptions only triggered the
uncaughtExceptionhandler if it was truly an uncaught thrown error. Right now in Restify 7, if you turn onhandleUncaughtExceptionsand add a listener for the event, all errors fromnext(err)and internal errors likeMethodNotAllowedwill trigger theuncaughtExceptionhandler.