9

I'm having trouble understanding how to handle something that it seems like would be a pretty basic aspect of express. If I have some code that throws an exception in an async callback, there is no way I can catch that exception because the try/catch block is no longer in scope by the time the callback is running. In these scenarios the browser will hang until it eventually give up stating that the server is unresponsive. This is a very bad user experience. I would much rather be able to immediately return a 500 error to the client. The default express error handler apparently does not handle this situation. Here is some sample code:

var express = require("express");

var app = express();
app.use(app.router);
//express error handler (never called)
app.use(function(err, req, res, next) {
    console.log(err);
    res.send(500);
});

app.get("/test", function(req, res, next) {
    require("fs").readFile("/some/file", function(err, data) {
        a.b(); //blow up
    });
});

app.listen(8888);

In the above code, the line a.b() throws a "ReferenceError: a is not defined" exception. The defined error handler is never called. Notice that the err object returned by fs.readFile() is null in this case because the file was correctly read. The bug is the code inside the async handler.

I have read this post about using node's uncaughtExpception even, but the documentation says not use that method. Even if I did use it, how would I then send the 500 response back to the user? The express response object is no longer around for me to use.

So how do you handle this scenario?

Community
  • 1
  • 1
d512
  • 32,267
  • 28
  • 81
  • 107
  • Why would the error be thrown in the callback? More likely, MongooseObject would throw an error internally, where you would catch it, and then `callback(err)`. Now the callback shown above has a falsy error object and you can send the 500 response. – Plato Aug 23 '13 at 17:44
  • If Mongoose gets into situations where it does NOT callback with an error... hmm I dunno perhaps you could make a setTimeout wrapper. but I would expect all errors to be immediately passed to the callback – Plato Aug 23 '13 at 17:45
  • It's not Mongoose I'm worried about, I was just using that as a way to call an async function. I'm more worried about my own code doing something dumb and causing an exception to be thrown. I'll update the sample code to make that more clear. – d512 Aug 23 '13 at 17:47
  • why not put the try/catch block inside the callback, wrapping the part of the code that you are concerned about? are you looking for a global catchall to avoid doing that everywhere? – Plato Aug 23 '13 at 17:52
  • Yes, I don't want to put try catches in every single async block of code that I have, which would be hundreds. Putting it in every top level express handler would probably be okay though. – d512 Aug 23 '13 at 17:58

2 Answers2

7

OK, I'm just going to post an entirely different answer since my first answer has some valuable information but is off topic in hindsight.

The short answer: the correct thing to do is what already happens: your program should print a stack trace and exit with an error.

The underlying thinking:

So I think you need to think about errors in different categories. My first answer deals with data-related errors that a well-written program can and should handle cleanly. What you are describing is a CRASH. If you read the node.js documentation you linked to, it is correct. The only useful thing your program can do at this point is exit with a stack trace and allow a process supervisor to restart it and attain an understood state. Once your program has crashed, it is essentially unrecoverable due to the extremely wide range of errors that could be the root cause of an exception getting to the top of the stack. In your specific example, this error is just going to continue happening every time until the source code bug is fixed and the application is redeployed. If you are worried that untested and buggy code is going to get into your application, adding more untested and buggy error handling code isn't really addressing the right problem.

But in short, no, there's no way to get a reference to the HTTP request object that caused this exception so AFAIK you cannot change the way this is perceived by the end user in the browser, outside of handling this at an intermediate reverse proxy layer where you could configure a crude timeout and send a more friendly error page (which of course would be useless for any request that isn't for a full HTML document).

The Bible of Error Handling in Node

Error Handling in Node.js by Dave Pacheco is the definitive work on this topic in my opinion. It is comprehensive, extensive, and thorough. I recommend reading and re-reading periodically.


To address @asparagino's comments, if an unhandled exception is easily reproducible or happening with high frequency, it's not an edge case, it's a bug. The correct thing to do is improve your code to not generate uncaught exceptions in face of this situation. Actually handle the condition, thus converting a programmer error into an operational error, and your program can continue without a restart and without an uncaught exception.

Peter Lyons
  • 142,938
  • 30
  • 279
  • 274
  • Thanks for the answer Peter. This is what I suspected from what I've been reading. I'm not sure I agree 100% with the idea that the only useful thing a program can do in the face of an unexpected exception is to reboot though. Obviously from an end user perspective that's not a good experience. In addition to that, it would affect all other requests going on at that time. I've written plenty of web apps that utilize top level exception handlers to return a 500, log the error, but keep the service up and running. Anyway, it works the way it works, I'll just have to deal with it. Thanks again. – d512 Aug 26 '13 at 20:26
  • Yeah I think this is mostly an unavoidable consequence of event based IO. JavaScript's uber-dynamic nature also makes it easier for bugs to get into production. Maybe other answers will surface with some good suggestions. – Peter Lyons Aug 26 '13 at 21:09
  • 4
    @PeterLyons - A node process will typically be handling dozens or even thousands of concurrent requests. Each of these can contain valuable data and there will be a cost if it doesn't complete. You're building a juggler. Should it drop all the balls as soon as it drops one? Errors and exceptions should be logged. Logs should be reviewed and bugs and edge conditions tracked down and fixed. The process should not quit. – asparagino Jan 27 '15 at 23:44
  • Graceful handling of existing requests is sometimes possible, sometimes not. It's fine to close your http server so new requests stop arriving and have a brief grace period so current requests can complete if possible, then exit and restart. The point is continuing to operate indefinitely is going to make things worse. And if the uncaught exception affects every request, there's nothing you can do anyway to handle that cleanly. – Peter Lyons Jan 28 '15 at 03:14
  • @asparagino I also urge you to read [Error handling in node.js](https://www.joyent.com/developers/node/design/errors), particularly the section entitled "(Not) Handling Programmer Errors" where he explains that trying to process outstanding requests could actually misbehave and be worse than simply dropping them. – Peter Lyons Jan 28 '15 at 03:21
  • 1
    @PeterLyons - I think there are a potentially a lot of variables and that a flat "quit on exception" dogma is wrong for quite a few applications, including some I work on. In our scenario, after restarting a high traffic application after experiencing an edge condition, we'd likely experience it again in fractions of a second once we started receiving data again. Constantly restarting processes would just make us more, rather than less, broken and turn a fractional failure into a catastrophic one. I think we're probably in agreement that "it depends what you're doing". – asparagino Jan 30 '15 at 21:58
  • See my updated answer. That's not an edge case, it's a bug. Just fix it. – Peter Lyons Jan 30 '15 at 22:31
  • I agree with @asparagino , the problem is that its harder to find the bug when the process quites. A crash also causes all routes on the router to fail, thus creating more problems then necessary, – Daniel Kobe Sep 13 '17 at 01:45
5

You should use express's error handling middleware via app.use(error, req, res, next). Express maintains a separate middleware stack that it uses when the normal middleware stack throws an uncaught exception. (Side note that express just looks at the callback's arity (number of expected arguments) to categorize it as regular middleware or error handling middleware, which is a bit magical, so just keep in mind you must declare the arguments as above for express to understand this is an error handling middleware).

And based on your question and comments, just understand that exceptions aren't all that useful in node.js because every async call gets a new stack, which is why callbacks are used everywhere and the 1st argument is an error universally. Your try/catch block in the example is only going to catch an exception thrown directly by findById (like if id were undefined, as it is in your snippet), but once an actual call to the database is made, that's it, the call stack is over and no further exceptions can happen until a totally different call stack starts when node invokes the async IO callback.

Thanks for the answer, but this only works if I put the try/catch inside the async callback and have the catch do next(exp). I'd like to avoid having separate try/catch blocks in every single async callback.

Nope, that's not true. You don't have to manually call next(exp). Express will catch the error and trigger the error handling middleware for you (that's how express does it's developer-friendly exception report pages in dev mode anyway). And async libraries don't throw exceptions even under "normal" error conditions. They pass an error to the callback, so in general you don't have to bother with try/catch that much in node. Just never ignore an error parameter passed to a callback function, and you're fine.

You don't see this boilerplate in node:

someDb.query(someCriteria, function (error, result) {
  try {
    //some code to deal with result
  } catch (exception) {
    callback(exception);
  }
});

You do see this though:

someDb.query(someCriteria, function (error, result) {
  if (error) {
    callback(error);
    return;
  }
  //some code to deal with result
});

Node handles IO differently, which means the call stack works differently, which means exceptions work differently, which means error handling works differently. You can write a stable node/express app that handles errors without crashing without writing a single try/catch. (express has one that handles uncaught errors that bubble all the way to the top). It's not a functional limitation, it's just a consquence of async IO that means you have to write your code differently and handle errors with callbacks instead of exceptions. Thinking of it as a "limitation" as opposed to the "way it is" is putting a negative connotation on something that is really just a technical reality. There are clean and robust patterns for exception handling in both a synchronous and asynchronous paradigm.

Peter Lyons
  • 142,938
  • 30
  • 279
  • 274
  • Thanks for the answer, but this only works if I put the try/catch inside the async callback and have the catch do next(exp). I'd like to avoid having separate try/catch blocks in every single async callback. – d512 Aug 23 '13 at 18:11
  • Yeah, I think I understand the issue, I just don't know what to do about it. At some point, the code is going to throw an exception and I don't want the node process to crash or hang. Is this just an inherent limitation of node? – d512 Aug 23 '13 at 18:14
  • Peter, thanks for taking the time to post thorough answers. Either I am not explaining my problem quite right or I am doing something wrong in my error handling code. You say "Express will catch the error and trigger the error handling middleware for you" but I can't seem to make this work. I revamped my question to contain a simple yet complete example that demonstrates what I'm seeing. – d512 Aug 26 '13 at 04:48
  • Up for posing useful info. – d512 Aug 26 '13 at 20:27