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

Error handling documentation is incomplete. #466

Closed
ericelliott opened this issue Oct 17, 2013 · 26 comments
Closed

Error handling documentation is incomplete. #466

ericelliott opened this issue Oct 17, 2013 · 26 comments

Comments

@ericelliott
Copy link

Edit / resolution

The documentation about error handling is incomplete and confusing. Maybe it would help to explicitly clarify the following points in the section on error handling:

  1. Handle your errors immediately, where they're generated. next(err) really means res.send(err). It's not an alternative to throw err. If you need to log and shut down, pass it off to an error handling module instead of stuffing it in next(err) where it will be swallowed.
  2. server.on('uncaughtException'... is how you handle any errors that were thrown in middleware / routes. It works fine for that purpose, as long as you remember the first guideline. 'uncaughtException' will not be triggered for next(err) calls.
  3. formatters are the way to customize error messages going out to the users. I don't think they should be used for the other tasks described in this thread. They're definitely not a viable alternative to dealing with errors in uncaughtException, because presumably, once you get there, you're sending the user the error you want them to see... not necessarily the error that was originally generated. See point 1.
  4. For errors unrelated to middleware, remember to use process.on('uncaughtException'...

Here's the original issue, for posterity:

Problem

Restify swallows all errors coming from route handlers. It does invoke its own error handler, but that handler never bails, regardless of whether or not an error is recoverable.

Expected

It should be up to the application developer to determine whether or not a service restart is needed. While it should be safe to swallow 4xx errors, other types of unhandled errors could leave the application in undefined state, which could lead to all sorts of trouble.

Reproduce

Restify v2.6.0
Node v0.10.20

var restify = require('restify'),
  app = restify.createServer(),
  errorHandler = require('../error-handler.js'),

  handleError = function handleError(err) {
    console.log('Caught error!'); // never happens
    setTimeout(function () {
      process.exit(1);
    }, 3000);
  },

  middlewareError =
      function middlewareError(req, res, next) {
    throw new Error('Random middleware error.');
  };


app.get('/err', function (req, res, next) {
  // This doesn't get caught.
  next( new Error('Random unrecoverable error. ' +
    'Server is now running in undefined state!') );
});

app.get('/throwerr', function (req, res, next) {
  // This doesn't get caught.
  throw new Error('Random unrecoverable error. ' +
    'Server is now running in undefined state!');
});

// This gets caught, yay!
app.use(middlewareError);

app.get('/middleware', function (req, res, next) {
  // Placeholder to invoke middlewareError.
});

// This works for middleware. Fails for routes.
app.on('uncaughtException', handleError);

// Nope. This doesn't help.
process.on('uncaughtException', handleError);

app.listen(3000, function () {
  console.log('Listening on port 3000');
});
@gdbtek
Copy link

gdbtek commented Oct 17, 2013

@dilvie , can you point to your error-handler.js lib?

@ericelliott
Copy link
Author

Sure.. for reference, it's at https://github.com/dilvie/express-error-handler -- I was trying to add Restify support when I found this bug.

@ericelliott
Copy link
Author

Here's a usable workaround... but the fact that unhandledException does not work for route handlers should either be corrected, or documented with big red letters:

app.on('after', 
    function (request, response, route, err) {
  if (!err) {
    return;
  }

  handleError(err);
});

@ericelliott
Copy link
Author

Still trying to get my error handlers to play nice with restify. Here's some of the behavior that I've discovered through trial and error. I'd love to get some help making any sense of this seemingly inconsistent behavior. It's not at all clear to me at this point how to specify consistent custom error behavior for restify:

'use strict';

var restify = require('restify'),
  server = restify.createServer(),
  errorHandler = require('../error-handler.js'),
  restifyHandler = errorHandler.restify,

  handleError = restifyHandler({
    server: server
  }),

  middlewareError =
      function middlewareError() {
    throw new Error('Random middleware error.');
  };


server.get('/err', function (req, res, next) {
  // This doesn't get caught.
  next( new Error('Random unrecoverable error. ' +
    'Server is now running in undefined state!') );
});

server.get('/thrower', function () {
  // This doesn't get caught.
  throw new Error('Random unrecoverable error. ' +
    'Server is now running in undefined state!');
});

server.use( middlewareError );

server.get('/middleware', function () {
  // Placeholder to invoke middlewareError.
});

// This is supposedly the alternative to
// express.use(function (err, req, res, next));
// 
// However, it doesn't get called consistently
// like the express error middlewares do. See
// server.on('after'...) comments.
// 
// Without this, middleware errors get swallowed.
// 
// Question:
// * Why isn't this triggered for 404? How do we override
// built-in 404 handler?
server.on('uncaughtException', function (req, res, route, err) {
  console.log('======= server uncaughtException');
  res.send(200, { handler: 'server uncaughtException'});
  if (err.status <= 399 && err.status >= 500) {
    process.nextTick( process.exit(1) );    
  }
  // handleError(req, res, route, err);
});

// Errors sent to next(err) get swallowed if this
// isn't here.
// 
// Issues:
// * No err object on 404
server.on('after', function (req, res, route, err) {
  console.log('==== after', route, err);
  err = err || {};

  if (err.status <= 399 && err.status >= 500) {
    process.nextTick( process.exit(1) );    
  }

  // Don't try to send headers twice -- you don't
  // have to worry about this in Express error
  // handlers.
  if (!res.headersSent) {
    res.send(200, { handler: 'after'});
  }
  // handleError(req, res, route, err);
});

// This is necessary for everything that isn't
// route / middleware related.
process.on('uncaughtException', function (err) {
  console.log('==== process uncaughtException');
  err = err || {};
  console.log('======== ', arguments);
  if (!(err.status >= 400 && err.status <= 499)) {
    process.nextTick( process.exit(1) );    
  }
});

server.listen(3000, function () {
  console.log('Listening on port 3000');
});

By way of contrast, Express makes in trivial to set up consistent custom errors handlers for middleware and route handlers.

@ericelliott
Copy link
Author

A workaround for the 404 override issue:

server.get('/.+', errorHandler.httpError(404) );

But it's weird. There's no server.all() like there is in Express, so you have to do this a bunch of times if you want it to apply to every method.

@mcavage
Copy link
Contributor

mcavage commented Oct 22, 2013

restify is not express. To do what I think you're trying to do, you need to setup a formatter that responds the way you want, and listen for uncaughtException. There is no need to listen for after:

Here's a tweaked version of your script that works as expected. Note that next(new Error()) is explicitly not an unexpected error - that's part of the API so you can shorthand (essentially: res.send(500, ...); next(false);).

var assert = require('assert');
var restify = require('restify');

(function main() {
    var app = restify.createServer({
        formatters: {
            'text/plain': function (req, res, body) {
                var ret;
                if (body instanceof Error) {
                    if (/unexpected/.test(body.message)) {
                        res.statusCode = 500;
                    } else {
                        res.statusCode = 400;
                    }

                    ret = body.stack;
                } else if (Buffer.isBuffer(body)) {
                    ret = body.toString('base64');
                } else if (typeof (body) === 'object') {
                    ret = JSON.stringify(body, null, 2);
                } else {
                    ret = body;
                }

                return (ret + '\n');
            }
        }
    });

    app.get('/err', function (req, res, next) {
        // This is "normal behavior" -- returning next(Error)
        // is not an uncaught error. Your app explicitly told
        // restify to break the chain and return an error to the
        // user, where the response is formatted with a formatter
        next(new Error('normal error'));
    });

    app.get('/throwerr', function (req, res, next) {
        // This is an uncaught error that will propagate to the
        // restify server event
        throw new Error('unexpected route error');
    });

    // This gets caught, yay!
    app.use(function middlewareError(req, res, next) {
        throw new Error('unexpected middleware error');
    });

    app.get('/middleware', function stub(req, res, next) {
        next();
    });

    // This works for middleware. Fails for routes.
    app.on('uncaughtException', function (req, res, route, err) {
        console.error('uncaughtException: %s', err.stack);
        res.send(err);
    });

    app.listen(0, '127.0.0.1', function () {
        console.log('Listening at %s. Running tests...', app.url);
        var client = restify.createStringClient({
            url: app.url
        });

        var done = 0;
        function cb() {
            if (++done === 3) {
                client.close();
                app.close();
            }
        }

        // Expected to return a 400 "normal error"
        client.get('/err', function (err, _, res) {
            assert.ok(err);
            assert.equal(res.statusCode, 400);
            assert.ok(/normal/.test(err.message));
            cb();
        });

        // Expected to return a 500 "unexpected route error"
        client.get('/throwerr', function (err, _, res) {
            assert.ok(err);
            assert.equal(res.statusCode, 500);
            assert.ok(/unexpected route/.test(err.message));
            cb();
        });

        // Expected to return a 500 "middleware error"
        client.get('/middleware', function (err, _, res) {
            assert.ok(err);
            assert.equal(res.statusCode, 500);
            assert.ok(/unexpected middleware/.test(err.message));
            cb();
        });
    });
})();

@ericelliott
Copy link
Author

@mcavage Thank you for the clarification. However, even after spending many hours reading the docs and trying to figure out how all this is supposed to work, I still felt the need to come here and open this thread. Maybe you should consider this a bug against the documentation.

I have been working with several restify services, some written by me, some written by other people. I haven't seen any yet that have properly dealt with unrecoverable errors that were triggered in the middleware and routes. Can you point to any good examples?

I think a much larger section on formatters, specifically as they relate to error handling would be appropriate for the documentation.

I've updated the issue to reflect that documentation needs improvement.

@mcavage
Copy link
Contributor

mcavage commented Oct 22, 2013

Yes, agreed on documentation flaws -- restify lacks in a lot of places on docs, this is one of them for sure.

As to restify examples that are good, most of the ones I would point at are unfortunately not open source. But basically:

  • Use formatters to filter what is shown to end users -- all errors that are res.send() or next go through this.
    -- ^^ is not a place to handle state manipulation/etc -- think of it like a "view"
  • restify will handle all uncaught errors by simply setting the code to 500 and then invoking res.send(); if you want to override that behavior, use server.on('uncaughtException').
  • You basically don't want to be throw'ing in restify/node anyway; that's where domains are leveraged, and frankly they're dirty and don't always handle all clients (redis is notoriously bad at this).

Make sense?

@ericelliott
Copy link
Author

Here's the restify example code from express-error-handler (ignore the name... I chose it before I knew I'd need restify support):

'use strict';

var restify = require('restify'),
  server = restify.createServer(),
  errorHandler = require('../error-handler.js'),

  handleError = errorHandler({
    server: server,

    // Put the errorHandler in restify error
    // handling mode.
    framework: 'restify'
  }),

  // Since restify error handlers take error
  // last, and process 'uncaughtError' sends
  // error first, you'll need another one for
  // process exceptions. Don't pass the
  // framework: 'restify' setting this time:
  handleProcessError = errorHandler({
    server: server
  }),

  middlewareError =
      function middlewareError() {
    throw new Error('Random middleware error.');
  };

// Don't do this:
server.get('/brokenError', function (req, res, next) {
  var err = new Error('Random, possibly ' +
    'unrecoverable error. Server is now running ' +
    'in undefined state!');

  err.status = 500;

  // Warning! This error will go directly to the
  // user, and you won't have any opportunity to
  // examine it and possibly shut down the system.
  next(err);
});

// Instead, do this:
server.get('/err', function (req, res) {
  var err = new Error('Random, possibly ' +
    'unrecoverable error. Server is now running ' +
    'in undefined state!');

  err.status = 500;

  // You should invoke handleError directly in your
  // route instead of sending it to next() or 
  // throwing. Note that the restify error handler
  // has the call signature: req, res, route, err.
  // Normally, route is an object.
  handleError(req, res, '/err', err);
});


// This route demonstrates what happens when your
// routes throw. Never do this on
// purpose. Instead, invoke the
// error handler as described above.
server.get('/thrower', function () {
  var err = new Error('Random, possibly ' +
    'unrecoverable error. Server is now running ' +
    'in undefined state!');

  throw err;
});

// This demonstrates what happens when your
// middleware throws. As with routes, never do
// this on purpose. Instead, invoke the
// error handler as described above.
server.use(middlewareError);

server.get('/middleware', function () {
  // Placeholder to invoke middlewareError.
});

// This is called when an error is accidentally
// thrown. Under the hood, it uses Node's domain
// module for error handling, which limits the
// scope of the domain to the request / response
// cycle. Restify hooks up the domain for you,
// so you don't have to worry about binding
// request and response to the domain.
server.on('uncaughtException', handleError);

// We should still listen for process errors
// and shut down if we catch them. This handler
// will try to let server connections drain, first,
// and invoke your custom handlers if you have
// any defined.
process.on('uncaughtException', handleProcessError);

server.listen(3000, function () {
  console.log('Listening on port 3000');
});

Does that look like an accurate description of best practices for error handling in restify?

@micahr
Copy link
Member

micahr commented Jun 17, 2015

A little late, but: server.on('NotFound', function(req, res, err, next){}) is the best way to handle routes that aren't matched by any existing routes.

Closing as the documentation has been updated slightly in the 1.5 years since the last update.

@micahr micahr closed this as completed Jun 17, 2015
@Zertz
Copy link

Zertz commented Sep 15, 2015

@micahr Just to confirm, there is no way for the developer to override next(err) so it isn't swallowed by restify?

@DonutEspresso
Copy link
Member

Nexting with an err doesn't get swallowed - it's just shorthand for res.send(err).

@Zertz
Copy link

Zertz commented Sep 15, 2015

@DonutEspresso Thanks for the quick response!

There is a library for automatically generating REST endpoints from mongoose models, with support for both express and restify. Here's an example: https://github.com/florianholzapfel/express-restify-mongoose/blob/master/lib/express-restify-mongoose.js#L158

I'm trying to figure out if there a way to call next(err) and handle it similarly to express rather than having it call res.send(err) immediately?

@DonutEspresso
Copy link
Member

You can use the error events to handle errors on a per type basis:

function (req, res, next) {
    return next(new NotFoundError('file not found!');
}

server.on('NotFound', function(req, res, err, next) {
   // handle your error here
});

It does mean you have to be very explicit about your error types. There's ongoing work right now to support a generic catch all handler over in #892 , allowing you to handle all error types:

server.on('restifyError', function(req, res, err next) {
    // handle all errors passed to next here, whether it's Error or NotFoundError or anything that is an instance of Error
});

@Zertz
Copy link

Zertz commented Sep 15, 2015

@DonutEspresso That would be awesome! For us it simply becomes a matter of reordering parameters internally and exposing a standard interface.

@DonutEspresso
Copy link
Member

Cool - keep your eyes peeled, it's looking likely that this stuff will land in the next major release along with the breaking out of all the plugins, clients, and error modules.

@Zertz
Copy link

Zertz commented Sep 15, 2015

Do you have a rough ETA?

@micahr
Copy link
Member

micahr commented Sep 16, 2015

No specific timeline, but hopefully the next week or two.

@chbrown
Copy link

chbrown commented Oct 30, 2015

Is there a proper way to do this yet? The aforementioned #892 is open, but this issue is closed, so maybe there is an alternate solution?

@DonutEspresso
Copy link
Member

Sorry all - this fell off my radar. Will try to get this merged into 5.x branch, but a proper release is probably still a bit out.

@Anaphase
Copy link

Any update on this? It would be great if Restify supported connect-style error middleware, or at least had it documented somewhere that it doesn't.

@Zertz
Copy link

Zertz commented Jul 27, 2016

@DonutEspresso Is there anything we can do to help?

@pourquoi42
Copy link

I am trying to use restify with swagger node. For some requests, I get validation errors and I have no clue how and where to catch them. I would really appreciate a way to catch all errors and handle them, instead of spending hours on trial and error.

@DonutEspresso
Copy link
Member

@Anaphase - when you say connect style error middleware, what behavior are you expecting?

Also, would appreciate if we could move discussion to a new issue, as this thread is pretty old and we have brand new docs site being written up right now.

Here's a new section on errors being released with the new docs:
https://github.com/restify/node-restify/blob/5.x/docs/pages/components/server.md#errors

@Anaphase
Copy link

Anaphase commented Aug 5, 2016

@DonutEspresso, by "connect style error middleware" I mean the 4-argument middleware described here. Basically, if your middleware function has four arguments, it's treated as an error handler. I guess the Restify equivalent would be the restifyError event, with the exception that after the restifyError event, the error is just sent instead of processing the rest of the middleware in the chain. (Unless I'm wrong about that behavior.)

@DonutEspresso
Copy link
Member

You can set your own custom content in the event handlers (including restifyError event) by doing something like this:

server.on('InternalServer', function(req, res, err, callback) {
    err.body = 'Sorry, an error occurred!';
    return callback();
});

Alternatively, if you don't set body, then restify will just flush a serialized error out to the client.

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

10 participants