[Discuss] Resolving the unhandled rejections issue

[jasnell]

@nodejs/tsc @nodejs/collaborators (yes, pinging @nodejs/collaborators is intentional and yes, I know it causes lots of people to get notifications... that's what I want because I want broad input into this conversation)

We currently have two PRs open that deal with unhandled promise rejections.

• process: add multipleResolves event #22218
• util: expose promise events #21857

Both do or assume several things:

1. Both introduce APIs that users may use to observe Promise state.
2. Both make assumptions (albeit very different assumptions) about what the proper handling of unhandled rejections by Node.js should be.

These PRs have been generally treated as mutually exclusive by the authors and the discussion about the introduced APIs has been generally held up by disagreements in (a) the fundamental handling of unhandled rejections and (b) whether or not Node.js should warn users about potentially problematic coding patterns. Unfortunately, what I see happening now is that issues that should be separate threads of discussion are being lumped all in together. So I'd like to try to separate these things out.

Warn or Not Warn

Let's take the whole discussion about Promises out of the picture for now. There is a general philosophical/values discussion to be had about whether or not it is Node.js' responsibility to warn users about potentially problematic coding patterns that may exist. It has been argued by a couple of folks that it is not Node.js' place to "judge" someones code. This has been expanded to mean that when the user runs some code that is not ideal but otherwise runs, Node.js should just stay silent on the matter.

An example of this is resolving a Promise twice... e.g.

new Promise((resolve, reject) => {
  resolve('A')
  resolve('B')
});

or 

new Promise((resolve, reject) => {
  reject(new Error('A'));
  resolve('B');
});

In both cases, the resolve('B') is extraneous and will be swallowed by default by V8. The code will run but the second resolve is effectively a non-op. In a handful of cases, this situation could be intentional or could be a legitimate coding bug. Take the following, for instance... a forgotten break statement leads to the swallowed resolve:

let foo = getFoo()
new Promise((resolve, reject) => {
  switch (foo) {
    case 'A': resolve('A')
    case 'B': resolve('B')
  }
});

When we look at this, we can likely reason that this is clearly a programmer error, but the code will run just fine.

Thanks to an internal callback API provided to us by V8, Node.js can detect when this secondary resolve or rejection occurs. The question is, should Node.js, by default:

1. Emit a process warning letting the user know that this situation occurred, or
2. Invoke an optional callback API that the user must explicitly opt in to using to be notified about this situation
3. Both

Note that when the process.emitWarning() API was added back in the Node.js 6.0, I took the approach of allowing both. By default, process warnings are emitted to stderr. Usercode can, however, listen to the process.on('warning') event and use the --no-warnings command-line flag to disable the default handling and provide their own handling of the process warning events. Because runtime deprecations now use the process warning mechanism, runtime deprecation warnings can be handled in the same way.

For warnings about synchronous file system I/O, however, we take a different approach. Synchronous I/O can be a significant performance bottleneck in Node.js and is generally considered to be an antipattern, but there are perfectly legitimate use cases for it. Therefore, we do nothing by default when sync I/O is used but we provide an opt in command line flag --trace-sync-io that users can switch on to help identify where sync io may be happening within their applications.

Given precedent, I don't think we can argue that there is any single rule of thumb to be applied here. Whether or not we warn by default can really only be determined on a case by case basis determined by how useful, or how problematic, a given coding pattern may be. So with that, we need to determine if swallowed resolves/rejections are potentially problematic enough to warrant a warning by default.

Crash or Not Crash

When a Promise is rejected without a catch handler in place, what do we do? This has been a long raging discussion for several years now without any clear resolution. Some feel we should crash immediately, some feel we should ignore and just let it happen. Neither are wrong, neither are entirely correct. I would submit that this is something that Node.js should allow users to configure with a command line argument, but then the argument turns to what should a reasonable default among several options be. That's what we need to figure out here. For my money, crashing on garbage collection is not ideal but it's likely the best reasonable default option we have at this time so long as we also have the option to crash immediately or not at all available for users to select.

One API to Rule Them All

The two PRS currently open propose two different APIs users may use to get notified about Promise state.

The "swallowed resolves" PR (#22218) takes the approach of building on the existing process.on('unhandledRejection') event and adds a new process.on('multipleResolves') event.

For example:

function handler(type, p, reason, message) {
  // do something here
}
 process.on('multipleResolves', handler);

 new Promise((resolve, reject) => {
  resolve('A');
  resolve('B');
  reject(new Error('C'));
});

The "promise events API" PR (#21857) takes a more diagnostics API approach that adds a low level API for monitoring promise state. The basic idea would be to replace the existing process.on('unhandledRejection') event and to allow users to implement whatever handling they want:

const makeHook = (type) => (promise, value) => {
  // do something here
};
util.createPromiseHook({
  resolve: makeHook('resolve'),
  rejectWithNoHandler: makeHook('rejectWithNoHandler'),
  handlerAddedAfterReject: makeHook('handlerAddedAfterReject'),
  rejectAfterResolved: makeHook('rejectAfterResolved'),
  resolveAfterResolved: makeHook('resolveAfterResolved'),
}).enable();

const x = (async () => {
  throw new Error('A');
})();

Each of the APIs have their merits, and each have their warts. In the discussion threads for each of the PRs, the authors have argued their cases for why theirs in the right approach and we need to come to a conclusion on which API we want to move forward with. Given the disagreement thus far and the inability to come to resolution, the decision is likely to fall to the @nodejs/tsc unless the authors of the two PRs can come together and work through some compromise approach (which is what I would like to see).

Personally, I'm good with either of the two PRs. None of the arguments in favor of either has swayed me for or against either of them.

When discussing this issue, please separate these three individual aspects so that any one of them does not get lost in the discussion.

[mmarchini]

Also pinging @nodejs/diagnostics and @nodejs/post-mortem since there are interested parties there who are not core collaborators.

[mcollina]

Both PRs are fine code-wise.

I am in favour of warning and crashing. I think warning adding that warning is good. I've seen plenty of code that had very hard to spot bugs because of that. It is a very tricky situation. It also encourages bad practices, like on('data', resolve) (this is hitting the resolve  function for every chunk, while only the first is returned).

Node.js is built under the assumption that crashing in case of an uncaught exception is the right thing to do:

Note that 'uncaughtException' is a crude mechanism for exception handling intended to be used only as a last resort. The event should not be used as an equivalent to On Error Resume Next. Unhandled exceptions inherently mean that an application is in an undefined state. Attempting to resume application code without properly recovering from the exception can cause additional unforeseen and unpredictable issues.

Not doing this for promises is a real world problem in a lot of companies adopting Node.js for writing server-side applications. Note that the following code could introduce a memory or file descriptor leak in case of errors:

const express = require('express')
const controller = require('./controller')
const app = express()

app.get('/', async (req, res) => res.send(await controller(req)))

app.listen(3000, () => console.log('Example app listening on port 3000!'))

This is no different than:

const express = require('express')
const controller = require('./controller')
const app = express()

app.get('/', (req, res) => {
  controller((err, data) => {
    if (data) {
      res.send(data)
    }
  })
})

app.listen(3000, () => console.log('Example app listening on port 3000!'))

(this is not an express problem BTW, but I'm using express to showcase a very common pattern).

Considering how Node.js is built, crashing on 'unhandledRejection' is the safest thing to do by default to avoid leaks.

[misterdjules]

I've done some research internally at Netflix around this, and wrote a report about it. That survey doesn't map 100% to the questions in this issue but some of the conclusions do:

• No evidence that exiting on GCed unhandled promise is desirable
• Exiting on unhandled rejections could be well accepted by Node.js users

As a result of doing that survey my position is still that Node.js should provide a default of exiting on unhandled rejections (regardless of whether they are garbage collected) that is opt-out.

Please make sure to read the disclaimer before looking at this report.

I found collecting that data and doing interviews internally to be very helpful in trying to understand what is acceptable and unacceptable for users. I would really like if we could do a similar survey at the scale of the broader Node.js users community.

I know that nodejs/user-feedback#77 was started to do that, but it seems to struggle to get traction. I think if we went through that survey it could help us to get a better picture of what our users care about.

If there's a way I can help with that I'd be happy to continue contributing to that effort. /cc @nodejs/user-feedback

[devsnek]

@mcollina warnings in console don't work because they can't be revoked. you also can't revoke exiting the process. the unhandledRejection event makes no sense without the rejectionHandled event, and it annoys me that people ignore half of the API.

@misterdjules your survey is cool, but it still only represents 32 engineers at the same company working on the same infrastructure. We will definitely need more data to start drawing conclusions.

[misterdjules]

@devsnek

your survey is cool, but it still only represents 32 engineers at the same company working on the same infrastructure. We will definitely need more data to start drawing conclusions.

I think that's what the disclaimer that I linked to is saying, was that not clear when you read it?

I also mentioned that:

I would really like if we could do a similar survey at the scale of the broader Node.js users community.

So I think we're on the same page 👍

[mcollina]

@mcollina warnings in console don't work because they can't be revoked. you also can't revoke exiting the process. the unhandledRejection event makes no sense without the rejectionHandled event, and it annoys me that people ignore half of the API.

Over the last 2 years, in almost every consulting engagement I have been part of I had to fix significant production issues related to promise handling. This include potential DoS attack vectors. IMHO it is not safe to operate a Node.js application without a) a rigorous coding environment or b) an 'unhandledRejection' event handler that crashes the process.

I think Node.js should strive to be safer for newcomers.

It would be extremely easy for a developer to always .catch():

const express = require('express')
const controller = require('./controller')
const app = express()

app.get('/', wrap(async (req, res) => res.send(await controller(req))))

function wrap (func) {
  return function (req, res, next) {
    func(req, res, next).catch(next)
  }
}

app.listen(3000, () => console.log('Example app listening on port 3000!'))

The root for this problem is more deep, and it goes back to our callback model. Let's consider this little code:

stream.on('data', function (chunk) {
  doSomething(chunk, function onSomething (err) {
    if (err) {
      // do something with err!
      return
    }
    anotherFunction()
  })
}) 

Note that in that code, every exception that is thrown by anotherFunction() would crash the process by default.

Now a developer wants to migrate that to async/await and promises:

stream.on('data', async function (chunk) {
  try {
    await doSomething(chunk)
  }  catch (err) {
    // do something with err
    return
  }

  anotherFunction()
}) 

Note that in that code, every exception that is thrown by anotherFunction() is very likely to not clean up some system resources properly.

The problem is that a significant number of developers would "add" an async in front of a function without considering that it changes significantly how that function behaves. They are just doing it because they want to have await. I have seen this happening all the time, even by experience developers.

[devsnek]

@mcollina

I think Node.js should strive to be safer for newcomers.

I agree, of course. I want to make sure whatever we expose doesn't confuse newcomers any more than they already are by being inconsistent or breaking valid patterns of programming with JavaScript.

The problem is that a significant number of developers would "add" an async in front of a function without considering that it changes significantly how that function behaves.

So maybe we should be looking into developer education instead of putting training wheels on features by default. If I want to use a valid model of programming in JavaScript I shouldn't have to override node to do it.

We could even release an npm module to describe as many opinionated behaviours as we wanted. Honestly that would probably make the most sense, given how much of this is based around opinions around certain programming patterns. My or @BridgeAR's PRs would enable some pretty cool stuff to happen in userland, and I hope it would also fulfill your requirements, given how often you mention that things that can be done in userland should be done in userland.

Possible API:

require('@nodejs/promise-thing')({
  track: 'unhandled', // vs 'unhandled-on-gn'
  crash: true, // could emit an event or just log otherwise
});

P.S. only sith deal in absolutes 😉

[Qard]

I'm mostly in the fail-fast camp, thinking we should just crash when an unhandled exception occurs and provide a no-really-its-okay escape hatch for when you are really sure you know what you are doing. Approaching it the other way around just makes Node.js more and more a platform on which you are likely to shoot yourself in the foot. 🙀

[ronkorving]

On warnings:

• Don’t make users opt into a hundred different events. With every new "bad pattern" that gets identified, we’re going to get a new event to listen to, and we all have to collectively update a million repositories to deal with them. Please, no. The single "warning" event is fine though, since you can opt-in to that, log the warning, and either ignore it or shutdown. Yes, the event is less specific, but people really don't want to write 20 lines of boilerplate to deal with a ton of different granular events, that just get bigger and bigger with every Node release.
• ESLint is a thing, a very mature thing actually. You can detect use of Sync() functions with it for example, and I don’t see why we can’t detect double-resolve or resolve-after-reject patterns. People who care about proper, working, code, tend to use tools like ESLint already anyway. Why do this with runtime hooks when the whole thing can be avoided? So I say: don’t bother at runtime with "helpful" warnings like this, we have excellent tools (that can be improved if they lack a feature).

On crashing:

• Why treat unhandled rejections any different from uncaught exceptions? I never understood this. Consistency would be progress. Allow a handler, but if there is none, crash.

[devsnek]

ESLint

I couldn't agree more. +100000000

Why treat unhandled rejections any different from uncaught exceptions?

Promises are designed such that they don't halt execution when they reject. A camp of users (myself included) feels that by node shouldn't be infringing on JavaScript design goals by default.

I of course agree that we should have good debugging utilities, but I don't think they need to be killing processes and cluttering my console by default. Tooling like ndb and such can provide all the stuff we want better than core can do by itself, so I don't really understand why we wouldn't want to focus on making things easier for tooling, instead of trying to provide strange defaults.

[jasnell]

With regards to eslint, unfortunately it doesn't catch every case. For instance, it would not reliably be able to catch Matteo's `on('data', resolve)` example unless it's able to determine reliably that the thing you're using is an event emitter and the specific event being handled can be emitted more than once. Linting is great, but it cannot reliably catch all of the problematic cases.

…

On Wed, Sep 12, 2018, 20:48 Ron Korving ***@***.***> wrote: On *warnings*: - Don’t make users opt into a hundred different events. With every new "bad pattern" that gets identified, we’re going to get a new event to listen to, and we all have to collectively update a million repositories to deal with them. Please, no. The single "warning" event is fine though, since you can opt-in to that, log the warning, and either ignore it or shutdown. Yes, the event is less specific, but people really don't want to write 20 lines of boilerplate to deal with a ton of different granular events, that just get bigger and bigger with every Node release. - ESLint is a thing, a very mature thing actually. You can detect use of Sync() functions with it for example, and I don’t see why we can’t detect double-resolve or resolve-after-reject patterns. People who care about proper, working, code, tend to use tools like ESLint already anyway. Why do this with runtime hooks when the whole thing can be avoided? So I say: don’t bother at runtime with "helpful" warnings like this, we have excellent tools. On *crashing*: - Why treat unhandled rejections any different from uncaught exceptions? I never understood this. Consistency would be progress. Allow a handler, but if there is none, crash. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#22822 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAa2eZ87Hg4wGIb1azLQoPkBxFjdZA4Zks5uablhgaJpZM4WlodJ> .

[ronkorving]

@jasnell Excellent point. I guess then the question becomes: is the tooling good enough? IMHO, it's good enough to avoid having to make the case for a new dedicated event. I'll end my argument there :)