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] handleUncaughtExceptions incorrectly handles other types of errors #1709

Closed
3 tasks done
mridgway opened this issue Oct 26, 2018 · 1 comment
Closed
3 tasks done

Comments

@mridgway
Copy link
Contributor

  • Used appropriate template for the issue type
  • Searched both open and closed issues for duplicates of this issue
  • Title adequately and concisely reflects the feature or the bug

Bug Report

Restify Version

Restify 7.x

Node.js Version

Node 8.11.3

Expected behaviour

Turning on handleUncaughtExceptions should trigger uncaughtException handlers only for thrown exceptions that have not been caught.

Actual behaviour

Other exceptions like next(new Error()) or calling a method (like HEAD) that is not handled by the server trigger the uncaughtExceptions handlers.

Repro case

var restify = require('./');

var server = restify.createServer({
    handleUncaughtExceptions: true
});

server.on('uncaughtException', function exceptionHandler(req, res, route, err) {
    res.send(500, 'foo');
});

// Hitting this route incorrectly triggers the uncaughtException handler
server.get('/', function rootGet(req, res, next) {
    next(new Error('Test')); 
});

// Hitting this route correctly triggers the uncaughtException handler
server.get('/throw', function throwGet(req, res, next) {
    throw new Error('Thrown Error');
});

server.listen(3000);

// Hitting `HEAD /` incorrectly triggers the uncaughtException handler with `MethodNotAllowed` error

Cause

The logic for handling errors has been unified between uncaught exceptions and other types of route handling errors, but information on whether an error was uncaught is not retained.

Are you willing and able to fix this?

Yes, I have a branch ready at master...mridgway:fixUncaughtExceptionTrigger

@mridgway
Copy link
Contributor Author

Fixed in #1710

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant