Node-restify: how to handle uncaught errors?

[jhfoo]

• [ ] 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

It is not clear to me with the current version of restify how to handle runtime errors such as throw new Error(). I tried the following code but it did not catch:

server.on('uncaughtException', (req, res, route, err) => {
err.body = 'i caught it!';
res.send(500,err);
});

Can anyone advise?

Restify Version

5.2.0

Node.js Version

8.1.3

Expected behaviour

service respond 'I caught it'

This section details what you expected restify to do based on the code that you wrote

Actual behaviour

Service stops on runtime error

This section details what restify actually did when you ran your code

Repro case

Please include a simple and concise example reproducing this bug. Please _do
not_ just dump your application here. By either not providing a repro case or
by providing an overly complicated repro case, you are offloading the work of
isolating your bug to other developers, many of which are here voluntarily.
Good repro cases are single file Node.js applications, where the only logic
present is logic necessary to expose the undesired behaviour. You will often
find that when creating your repro case, you will solve the problem yourself!

Cause

If you have been able to trace the bug back to it source(s) in the code base,
please link to them here.

Are you willing and able to fix this?

yes

"Yes" or, if "no", what can current contributors do to help you create a PR?
If this issue is unique, as the checklist you completed above suggests, then
you are one of the few people who have encountered this bug in the wild.
While contributors will often help work on issues out of the kindness of
their hearts, its important to remember that you are the largest stakeholder
in seeing this bug resolved as you are the one experiencing it. Kindness
and contributions are what make Free Software go round, help pay it forward!

[souravdasslg]

Follow this Documentation http://restify.com/docs/4to5/
var server = restify.createServer({
handleUncaughtExceptions: true
});

[retrohacker]

Hey @jhfoo!

@souravdasslg is correct (:heart:), we no longer support keeping a server running after a thrown exception by default. Moving forward, handleUncaughtExceptions will more than likely be removed entirely since it relies on Node.js domains.

This behaviour is a bad practice in restify. Node.js has fairly fast start times and an uncaught exception is generally a sign of something fundamentally wrong with the service. When something causes an uncaught exception, it is usually best to throw away the process and start fresh.

In idiomatic restify, throw new Error() is usually reserved for cases in which you want the server to crash. next(Error()) and other forms of error handling are generally preferred. This allows you to differentiate between expected errors and unexpected errors.

Hope this helps :smile:

[jhfoo]

It makes sense to deprecate handleUncaughtExceptions if there is not alternative implementation beyond using Domains. I used server.on('restifyError') paired with next(new Error()) and it works. So thanks for pointing me to a longer-term solution!

I have 2 questions:

1. Should it be documented that the restifyError handler should not use res.send()?
2. Any reason why the entire err object in restifyError is sent as response, and not a specific attribute eg. err.body?

[jadidian]

@jhfoo I used to format errors like:

const server = restify.createServer({
  formatters: {
    'application/json': (request, response, body) => {
      if (body instanceof Error) {
        response.statusCode = body.statusCode || 500;
        return { message: 'Ooops...' };
      } else {
        // return body or whatever...
      }
    }
});

now that handleUncaughtExceptions option is deprecated, use next(new Error()) instead of throw new Error() in your handlers.

[DonutEspresso]

@jhfoo sorry for the delayed reply! To answer your questions:

1. restifyError can be used to intercept any error passed to next(err) before it gets flushed. So if you wanted to change the value of the Error message or body, you could do that there.
2. @jadidian has a great example here. Generally speaking, if you're using application/json as your response format, then restify will by default call JSON.stringify on the object (in this case, the Error). As an alternative, you can also define custom toJSON() implementations on your error object, which are eventually called by JSON.stringify.

[RobeeeJay]

We are now hitting this problem, triggered by a mysql deadlock error, when before it was caught and logged. I don't think it's a good idea for the service to die in this situation, it doesn't just take down the current connection, but all other connections the service is handling. :(

Yes, Node restarts fast, and in a production environment you would probably have multiple instances running. But a few hundred of your users getting a 500 because one of them somehow triggered an exception is perhaps not the solution. :/

A way to catch these outside of restify (not yet tested this in a production env) might be this:

process.on("uncaughtException", (options, error) => { if (error && error.stack) console.log("EXCEPTION:", error.stack); else console.log("EXCEPTION:", options); });

[retrohacker]

Hey @RobeeeJay,

Thanks for commenting :heart:

process.on("uncaughtException" is a similar approach to the underlying implementation that we are deprecating and removing. We were using domains. However you capture unhandled exceptions at the top level, note that your process is no longer in a known state. An expert from the documentation on domains

Warning: Don't Ignore Errors!

Domain error handlers are not a substitute for closing down a process when an error occurs.

By the very nature of how throw works in JavaScript, there is almost never any way to safely "pick up where you left off", without leaking references, or creating some other sort of undefined brittle state.

The safest way to respond to a thrown error is to shut down the process. Of course, in a normal web server, there may be many open connections, and it is not reasonable to abruptly shut those down because an error was triggered by someone else.

...

Logging the error and letting the process continue to run gives superficially great results, but is dangerous. Proceed with caution. This documentation seems to speak to your use case, perhaps there may be something helpful in there :-)

[RobeeeJay]

Logging the error and letting the process continue to run gives superficially great results, but is dangerous. Proceed with caution. This documentation seems to speak to your use case, perhaps there may be something helpful in there :-)

It depends a lot on your environment, for us in production an error is serious, we get pinged on slack and examine it quickly. An engineer is going to investigate every one and take necessary steps to prevent it happening again.

This is obviously different from scenarios where you may be unaware of the error for a significant time period.

My real issue however is the mysql lib throwing an exception, and this is probably not restify's responsibility. It would be nice to have the option to still catch it though, especially when you have the infrastructure to detect and recycle processes that have a memory leak automatically.

Because certainly for web apps, this is better than dropping multiple connections to clients. :(

[DonutEspresso]

Domains, while deprecated, are probably your best bet here. You can enable it by setting handleUncaughtExceptions: true when creating your restify server.

It's hard to make recommendations since everyone's appetite for fault tolerance is slightly different - but generally if a method/API you are calling can throw/errback (or is expected to throw/errback), you should handle it as close to the source as possible, and avoid letting it bubble up to something that doesn't know how to handle it (in this case, restify).

If you aren't expecting an error to be thrown, that would generally indicate either a problem in the method being called, or what should be an expected error that isn't being handled correctly.

A very similar use case is actually covered in Joyent's Error Handling document:

A database (or other) connection may be leaked, reducing the number of future requests you can handle in parallel. This can get so bad that you're left with just a few connections, and you end up handling requests in series instead of concurrently.

I don't know the exact nature of this mysql exception (expected? unexpected?), but the shared global state of JS makes it hard to make any guarantees upon an uncaught exception. Dropping connections to clients isn't great, but the above scenario may actually be worse, especially from an operational perspective.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[stale[bot]]

This issue has been automatically closed as stale because it has not had recent activity.