Skip to content
This repository has been archived by the owner on Nov 9, 2017. It is now read-only.

Error handling #173

Closed
ronkorving opened this issue Sep 1, 2017 · 10 comments
Closed

Error handling #173

ronkorving opened this issue Sep 1, 2017 · 10 comments

Comments

@ronkorving
Copy link

Hi everyone,

Following nodejs/node#15057, @benjamingr suggested I open an issue with the CTC regarding error handling. I am not sure if the solution of this lies in Node core, or in the bigger Node community.

It seems to me that for the user, errors can originate in these distinct areas:

  1. libuv
  2. Node.js
  3. Some external modules used by the application
  4. Some middleware library (if the application uses one)
  5. Some external modules used by the middleware
  6. Some assertion library used by the application
  7. Application code

The application developer only really controls the last one, but communication about errors to the end-user will often require a "code to human language"-translation. There are two ways that communication may happen:

  • Some middleware library will catch errors and send them to the user.
  • The application will catch errors and send them to the user.

Having a middleware take care of this is very appealing, because it can offer some guarantees that application code would not systematically have.

Regardless, a developer has to make a choice which errors are sent to the user, and in what shape. We will often want to log all stack traces, in which case that is the easy part. But when we want to send an error code to the user, we have some choices to make, with regards to which errors' codes we even want to consider. So how do we handle the errors the user should care about? (all other errors could fall back to a "generic server error" communication to the user)

  1. The application implements a special MyError class, which will be the only error type considered (instanceof or magic .isMyError property test).
  2. We simply communicate a special property on the error object (eg: .code) when it's there.
  3. Middleware wraps all errors into its own error class, and userland uses the same error class for its own errors.
  4. ... There may be some other ideas

error-type-testing

  • instanceof requires an error class to be public, which is often not the case (including Node's error classes)
  • Assertion libraries don't change their error types for the user and are often used cross-concerns (even Node's AssertionError is not a NodeError, which makes sense if the user used the assert module, but not when an assertion failed in Node).

special properties

  • .code (or whatever name the user may end up using) can inevitably come from anywhere. It's hard to make any guarantees about unique naming. If it didn't come from the right source, it may expose sensitive information to userland.
  • Assertion libraries don't tend to have an ability to set codes on errors, they only set the .message. If we were to use .message instead of .code, we risk even more sensitive information leaking to userland.

wrapping all errors into one error class

This is like Joyent's verror module. I actually think it comes closest to solving the issue. When a developer uses a middleware, that middleware will have to be verror-aware, and the developer themselves will have to use verror to wrap every single error that may originate from userland (the only thing missing there seems to be a distinct error code property to translate into human readable, as that more easily allows for localization). I think the challenge here is that its successful use depends heavily on a developer's discipline to use this everywhere. It also requires the developer to use an assertion library that throws VError objects, and for that assertion library to accept error-codes from the developer (not just human readable messages).

It seems to me (but I'm just one person) that the number of obvious, elegant solutions to the problem of error communication is roughly zero (although VError comes close). This problem is of course also not unique to Node. But I'm wondering what kind of ideas the bright minds of the community may have about dealing with this issue to solve all challenges for a large group of users. Is there any really elegant approach to this? Is there anything that can be done within Node core, perhaps by making something rather "official" for errors, that modules, middleware, and assertion libraries can embrace?

Thank you for your time.

@benjamingr
Copy link
Member

@petkaantonov

@benjamingr
Copy link
Member

instanceof requires an error class to be public, which is often not the case (including Node's error classes)

Can you give some examples of that?

Assertion libraries don't change their error types for the user and are often used cross-concerns (even Node's AssertionError is not a NodeError, which makes sense if the user used the assert module, but not when an assertion failed in Node).

I think it's important to acknowledge that there is no difference between a user model throwing an AssertionError and Node.js used inside a library doing so. The only location that can decide if an error is 'internal' is its consumer.

This is similar to programmer/operational errors - it depends on who is consuming the error and not who is producing it for the decision.

@jasnell
Copy link
Member

jasnell commented Sep 1, 2017

The closest thing we have so far is the effort to migrate all errors to our internal/errors mechanism which assigns a static .code property and consistent error messages. The effort thus far has been focused on JS errors, but I will soon be introducing a PR that provides a similar mechanism for C/C++ raised errors.

Aside from migrating the codes, I've been exploring the possibility of adding a new .info property (like that used in verror) whose value is a dictionary of contextual properties that are specific to the error.

I'm working through this incrementally because just the process of converting to the internal/errors mechanism has been rather slow and tedious.

@mcollina
Copy link
Member

mcollina commented Sep 4, 2017

I am a bit lost by the definition of middleware (as I think it means Express middlewere) here, and the fact that we are just focusing only on HTTP in your description. Node is used for all sort of purposes, so we should think more generically for this problem.

The high-level description is also missing logging as 1st class concern. How the Error is represented in a log line? Would the developer know what caused the problem? Is all the relevant information there and processable? Bunyan, Winston and Pino (and all the other loggers I know of) take all special care on how to process errors.

@ronkorving
Copy link
Author

@mcollina You're right, apologies, that is confusing. I've never gotten used to how "Express middleware" uses that word. To me that's not middleware. I meant it in the more traditional sense: an architecture or framework around which you build your software. Like LoopBack for example.

@ronkorving
Copy link
Author

@benjamingr

instanceof requires an error class to be public, which is often not the case (including Node's error classes)

Can you give some examples of that?

https://github.com/nodejs/node/blob/b24e269a482812193fb3cd8b6ced4477f8e5e1c5/lib/internal/errors.js#L84-L86

Those are not exposed to userland, are they?

@mcollina
Copy link
Member

mcollina commented Sep 5, 2017

boom is also a very popular library for Error handling that goes into this direction for the Hapi ecosystem.

@benjamingr
Copy link
Member

boom is also a very popular library for Error handling that goes into this direction for the Hapi ecosystem.

Interesting, pinging boom's @hueniverse to see if they have anything to add.

@ronkorving

Those are not exposed to userland, are they?

But a instanceof TypeError still passes on makeError(TypeError) - right?

@hueniverse
Copy link

Not sure what I can add to this conversation. I am not following what the actual need/proposal here is.

@Trott
Copy link
Member

Trott commented Sep 8, 2017

@ronkorving I know this issue is relatively recent, but I'm going to close it because recent governance changes make this repository obsolete. Feel free to re-open in another repository (maybe this one should go in the main node repo?) and link back to this issue so people have context. I'm sorry for the inconvenience.

@Trott Trott closed this as completed Sep 8, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants