Terminate process on unhandled promise rejection

[aduh95]

• Version: v11.0.0-nightly2018042865d97c96aa
• Platform: all
• Subsystem: Promise

When a Promise is rejected and has no catch handler, the following warning is displayed on stderr:

$ node -p "Promise.reject()"
Promise { <rejected> undefined }
(node:15518) UnhandledPromiseRejectionWarning: undefined
(node:15518) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). (rejection id: 1)
(node:15518) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.
$ echo $?
0

This behavior is in place for quite some time now I believe, and maybe it would be time to actually make node exit on a non-zero code.

[benjamingr]

@aduh95 thanks for your interest! there is ongoing discussion about this (from this week) in #20097

Your participation and review there are more than welcome. Please note that I've found it a rather complex subject that appears simple and I really want to make sure that we get it right.

• We can't just throw on it either due to the rejectionHandled event and legitimate use cases.
• We want to allow people to turn off unhandled rejection detection completely for language semantics.
• We can't just turn off the current "unhandledRejection" system since GC semantics for garbage collection have false negatives (where the promise is retained rejected in a global variable for instance)
• We need to enable people detect if an exception is from a rejection or not - or at least add somthing that allows us to achieve opt out.
• Performance needs to be reasonable and useful.

I believe the work @BridgeAR has done in #20097 (and the prior work by @Fishrock123 ) is excellent and we're moving towards a solution to all these problems:

• continue emitting unhandledRejection events and log but without the deprecation warning.
• exit the process if no unhandledRejection handler is attached and eventually if one is (with a migration path outlined in the PR).

When testing this a while back - it proved to be a significant improvement of UX.

[advanceddeveloper]

IMHO it would be better to throw an unhandled exception instead of immediatelly terminating the process. The exception could then be processed in process.on('uncaughtException') which is usually used for cleanup and similar stuff. Terminating the process disables the possibility of performing a cleanup before exit.

[benjamingr]

@advanceddeveloper

The exception could then be processed in process.on('uncaughtException') which is usually used for cleanup and similar stuff.

That's the plan, and what we decided to do together in #20097 - please consider going over the semantics there and leaving a comment - we are very open for feedback. Note it's a pretty lengthy read - sorry for that - I tried to outline most of it in #20392 (comment) but I missed some stuff like uncaughtException

[chk-]

Hi,
I'm not sure if this is the right thread for my issue, but I was searching for a place to discuss my afraidness about the deprecation warning from point of view of a consumer.
I have got some async tasks, which I compute in parallel and collect the promises in an array. Then with

Promise.all([...]).then(function(results) {
  ...
}).catch(function(err) {
  ...
});

I do some work, after all promise fullfilled and catch if one of the promises where rejected.
But I do get the deprecation warning with this coding stile and now am afraid, when time comes and my process gets terminated. I believe I catch the error and do not await an UnhandledPromiseRejectionWarning with a global catch.

[benjamingr]

Hi, it is impossible for the code above to generate an unhandled rejection since you are handling it (in a .catch).

Feel free to make a gist with an mcve and I promise to look into it.

In any case - terminating the process only happens on GC when Node can prove that the rejection will never be handled.

[chk-]

@benjamingr Hi, you're right. I checked this situation again and realized I missinterpreted it. Thank you for claryfying, that it is impossible! It makes future searches on deprectaion warning easier becaurse of knowing the promise chain has to be someware broken.

[mike-marcacci]

I want to add a quick note of caution before things proceed too fast. This breaks a really important feature of the Promise spec, which is the ability to defer handling until later. I understand the rationale behind the change, and I'm not really against it... but I think it's important to communicate how to work around this change.

There are many useful patterns that use deferred handlers, and they're employed across hundreds of packages. These will start crashing with this change, and I think it's important that maintainers can quickly fix these without scouring the internet.

I was writing demos here (as this issue was pretty easy to find), but it got a bit long, so I moved it to medium.

EDIT: This is particularly important for libraries that support both callback and (native) PromiseAPIs, as any app using callbacks will begin crashing on each rejection!

[BridgeAR]

@mike-marcacci i just read your article but this is not how things are planned to work in Node.js 11. It is not even clear if anything changes in 11 but one thing is clear: the default would not crash with your example. In this case nothing will change besides not receiving a deprecation warning anymore. The reason is that it is possible to either have false negatives or false positives and the default should be safe to use.

[benjamingr]

There are many useful patterns that use deferred handlers, and they're employed across hundreds of packages.

We've actually surveyed the ecosystem quite extensively and have found it to be used very rarely in the wild if at all. In fact, people seldom silence the unhandled rejection warnings at all - a lot of people just terminate the process on first unhandledRejection.

Also, like @BridgeAR said - none of your examples crash in Node.js 11, all of them work as you intend. Node.js 11 promises will (hopefully) crash on GC.

[mike-marcacci]

Hey @BridgeAR and @benjamingr – thanks for your replies. I deleted that post shortly after your comments to avoid sowing misinformation. I'm really glad to hear I had misinterpreted the deprecation message, but I'm quite sure I'm not alone in that interpretation.

Is the warning implemented differently than the planned crash? You mentioned that this is safe:

const promise = Promise.reject(new Error("Something happened!"));
setTimeout(async () => {
  // You want to process the result here...
  try {
    const result = await promise;
    console.log(`Hello, ${result.toUpperCase()}`)
  }
  // ...and handle any error here.
  catch (err) {
    console.error("There was an error:", err.message);
  }
}, 100);

...but it currently warns that this behavior is deprecated and will crash in future versions of node.

[benjamingr]

@mike-marcacci that's a bug in our current implementation that Ruben's work will solve.

Note that by all means you should not write code like this, the fact we alllow it to error on the safe side as a platform doesn't mean it's a good idea in general - it makes tooling a lot harder and it will still warn (just without the deprecation warning). This sort of code is risky IMO

[mike-marcacci]

@benjamingr - thanks for confirming this. I agree that my above code is totally unnecessary, but it illustrates the mechanics behind more complex patterns that are extremely difficult to implement without the Promise API.

While I totally believe you've done your due diligence in surveying the ecosystem, I personally have used these patterns in a number of different projects (both open and closed source), and there are cases where they will crash even on GC. Is my "hack" still a valid technique for bypassing this behavior when I'm absolutely sure it's the right thing to do?

const promise = Promise.reject(new Error("Something happened!"));
promise.catch(() => {})

I certainly don't want to promote an anti-pattern or mislead people into thinking that this technique is generally a good idea; but at the same time, I want to make sure developers are equipped to handle this change when it's appropriate.

[devsnek]

its worth noting that attaching catch to promises at any time is part of the design, not by accident. i wouldn't consider a rejection truly unhandled unless the promise was gc'd without being handled. (which i think is what @BridgeAR's pr does?)

[benjamingr]

Is my "hack" still a valid technique for bypassing this behavior when I'm absolutely sure it's the right thing to do?

Yes. That is entirely appropriate and the way to do a "late binding" of a catch handler. I have to say that I expected some 'noise' when we added the unhandled rejection warnings by default of people complaining about it and no one has. In practice almost no one does this (although like they are allowed). Like subclassing promises - this is something people can do but ended up not being worth it.

its worth noting that attaching catch to promises at any time is part of the design, not by accident.

I don't think we'd have this API if we did this today - things looked very different when the aplus spec was made. Node is entirely at liberty to limit the design the same way we crash the process on errors (which browsers do not).

which i think is what @BridgeAR's pr does?

That is correct

[benjamingr]

@flaviomx14 hey, this issue is not for support on Node.js problems. I recommend asking on https://github.com/nodejs/help/issues. Namely this issue tracker is for Node.js core development and not for support (there is a separate one for that which you're very welcome to use).

I'm going to go ahead and hide your comment as "off topic" and welcome you to post in nodejs/help. Cheers.

[kreig303]

Was going to file a separate issue but this one already existed...

Due to #26599 we now will have a flag going forward --unhandled-rejections with three levels: none warn and strict to allow for configuring the behavior. The default is to use warn, which is same as current behavior (I would expect none to be for people who know what they're doing!) whereas strict would produce an uncaughtException.

I am proposing that strict become the default at some point as even thought this changes the expected behavior it also discourages lazy coding or naive coding which I have seen confuse numerous engineers who were not privy to the current non-intuitive behavior.

@mcollina, consider this my follow-up. :-)

[devsnek]

i'm still not convinced that it is the runtime's job to deal with "lazy" or "naive" coding. i mean, it doesn't warn every time you use == or call hasOwnProperty directly on an object... i don't see how this is any different. the runtime should just be focused on being correct (and in this case dying on unhandled rejections would mean some valid js code wouldn't run in node by default, which seems undesirable)

[BridgeAR]

@devsnek

some valid js code wouldn't run in node by default, which seems undesirable

I disagree. Some times things that are possible should AFAIC only be possible as an opt-in. This is especially important since the language is used in lots of different contexts and as server-side language it's normally best to better be safe than sorry.

I believe we should have defaults that serve the majority of our users best and in this case I am very certain that most people prefer to use the strict version as default. That way less people have to make sure they actually change the default. Ideally we'll have a small survey for it to have actual proof for it.

I'll see if I add another short session for this topic at the collaborator summit.

[binki]

Sorry for the noise, but I do not know where the documentation for recommended patterns to avoid triggering the message falsely are. I am concerned that this pattern is still unsafe with the new behavior:

async function doSomethingAsync(x) {
    throw new Error(`fail ${x}!`);
}
(async () => {
  const doSomething0Promise = doSomethingAsync(0);
  const doSomething1Promise = doSomethingAsync(1);
  try {
    console.log(`result 0: ${await doSomething0Promise}`);
    console.log(`result 1: ${await doSomething1Promise}`);
  } catch (ex) {
    console.error(`failure: ${ex}`);
    process.exitCode = 1;
  }
})();

I value this pattern because it allows concise concurrency. I recognize that, in many cases, the rejections will not be “handled”. And the IIFE will exit without waiting for all promises to complete (making it different from Promise.all()) and I will only see the first awaited rejection/error (same as Promise.all()). But if I use it, I get the demonstrated rejections. Is there a solution for this pattern that doesn’t sacrifice concision or introduce complicated synchronization?

[Hakerh400]

@binki This allows concurrency, but waits for all promises to resolve/reject:

const all = ps => new Promise((res, rej) => {
  const arr = ps.map(() => null);
  let n = 0, ok = 1, err;
  ps.forEach((pr, i) => {
    pr.then(r => arr[i] = r)
      .catch(e => ok && (ok = 0, err = e))
      .finally(() => {
        if(++n === arr.length)
          ok ? res(arr) : rej(err);
      });
  });
});

const doSomethingAsync = async x => {
  throw new Error(`fail ${x}!`);
};

(async () => {
  let results = all([
    doSomethingAsync(0),
    doSomethingAsync(1),
  ]);
  try{
    results = await results;
    console.log(`result 0: ${results[0]}`);
    console.log(`result 1: ${results[1]}`);
  }catch(ex){
    console.error(`failure: ${ex}`);
    process.exitCode = 1;
  }
})().catch(console.log);

[benjamingr]

@blinki that code has a real big - if the second function rejects you don't handle the error which has big concequences

Consider Promise.all

[binki]

@Hakerh400 @benjamingr In both of your solutions, you change the behavior of my code. You force the first console.log(`result 0:…`) to wait for doSomethingAsync(1) to complete needlessly. Also, you’re sacrificing concision. The only difference between using Promise.all() (such as in a finally {…} block) (with all the pomp and circumstance doing so requires) and my code is that my code’s function completes its Promise prior to the completion of doSomethingAsync(1) (but only if doSomethingAsync(0) throws).

I can see how my function not waiting for all of the Promises it is responsible for to run to completion is an issue, however. It is what people expect and you might assume that, once a function completes its Promise, it will no longer be accessing a shared resource such as a passed-in object. So I will try to avoid that pattern even though it is easier to write and more concise.

[mike-marcacci]

Since this conversation has been hitting my inbox again, I want to reiterate how bad of an idea I think this change is. There are many scenarios in which a library might want to return a promise that the user may choose to handle or not.

• Many projects (including those I've authored) are designed to work with promise-based patterns and callback patterns; this change would cause crashes for anyone following the callback style, as the returned promises are unhandled.
• Many projects (including those I've authored) have functions that are "best attempt" asynchronous side effects, like logging or unlocking a resource, where the vast majority of use cases don't need to wait for a success or failure.
• Anywhere that user-land resource pooling exists, it is possible to discard unused promises that are processing something in the background. If this proposal is fully implemented, we will end up with this garbage everywhere.

Fundamentally, I think promise rejection is substantially different than "throwing" under normal synchronous flow.

Let's take this for example:

function a() { console.log("a"); return 1; }
function b(input) { throw new Error("oops"); return input + 2; }
function c(input) { console.log("c"); return input + 3; }

function run() {
  const resultA = a();
  const resultB = b(resultA);
  return c(resultB);
}

console.log("before");
const result = run();
console.log("after", result);

It's abundantly clear how the throw propagates. The behavior is deterministic and necessary, as we were unable to define resultB.

This scenario is very similar:

async function a() { console.log("a"); return 1; }
async function b(input) { throw new Error("oops"); return input + 2; }
async function c(input) { console.log("c"); return input + 3; }

async function run() {
  const resultA = await a();
  const resultB = await b(resultA);
  return c(resultB);
}

console.log("before");
const result = run();
console.log("before 2");

...but has a critical difference: the expression console.log("before 2"); does not and cannot depend on the resolved value result. The throw propagates through all chained promises, and when it stops, there is no remaining undefined behavior! No piece of code is left in an unclear state, and therefore there is no reason to crash.

To be perfectly frank, this proposal seems far more about creating the appearance of safety than addressing an actual deficit in application correctness. I'm not questioning the value in detecting unhandled promises (resolved OR rejected) as a development tool for calling attention to a potentially undesired flow... but just like other lint rules, this belongs in tooling and NOT the execution environment.

Just my 2 cents.

[vweevers]

Many projects (including those I've authored) are designed to work with promise-based patterns and callback patterns; this change would cause crashes for anyone following the callback style, as the returned promises are unhandled.

A better pattern is to not return a promise if a callback is passed in. If the internals themselves use a promise, do .catch(callback) and return void. If the internals use callbacks, there's no need to create a promise. Either way, there's no risk of an unhandled rejection.

[dmaevsky]

@targos This is a valid argument.
Well, I just really hope the true reason behind the push for the change is not the fact that Node just does not clean up its internals properly upon an unhandled rejection, as was mentioned in one of the comments above.
Personally I still feel that changing the default would just introduce new possibilities for the Node servers to crash in subtle and hard to detect ways, but I can of course live with a flag...

[mcollina]

@dmaevsky this PR #27867 goes into that direction, i.e. make Node clean up his internals in case of errors.

[dfabulich]

I think I've figured out a pretty good compromise solution in PR #33021. I invite even the most cantankerous among you to review it.

It defines a new default mode for --unhandled-rejections that throws exceptions on unhandled rejections by default, but allows userland code to set an unhandledRejections hook to override that behavior, so you don't have to mess around with CLI flags if you don't want to.

[ghost]

I have an idea: trigger a BSOD on unhandled promise rejection... just to teach users

[mmarchini]

TSC will be voting on the intended default behavior for unhandled rejections on v15, where we also intend to remove the deprecation warning. We want to listen to Node.js users experiences with unhandled promises and what you think should be the default before voting, so we prepared a survey: https://www.surveymonkey.com/r/FTJM7YD

We also wrote an accompanying blog post for extra context: https://medium.com/@nodejs/node-js-promise-reject-use-case-survey-98e3328340c9

The survey will run for at least two weeks, at which point we'll evaluate if the number of replies is enough for us to move forward, otherwise we might extend it for a week or two. Please fill out the survey as it will help us decide the future of unhandled promise rejections on Node.js!

[dfabulich]

@mmarchini I'm not sure if you have the power to fix this, but the Medium post has weird line breaks (presumably from being copied and pasted from plain text).

[klesun]

The survey seems to be closed, so I'll post my feedback here, hopefully you'll find it helpful:

Today I ran a project after updating nodejs to v15 and found out that:

const whenSomeOptionalAsyncData = getSomeOptionalAsyncData();
...
return {
  getSomeOptionalAsyncData: () => whenSomeOptionalAsyncData,
}

terminates my application (moreover without a clear stack trace, as getSomeOptionalAsyncData() is a library function)

I fail to understand what is wrong with such coding pattern to require terminating the application: the API allows you to either retrieve the cached data if it was fetched successfully or get a rejection with explanation of why it wasn't. To preserve such behaviour while complying to the new unhandled promise shutdown, I would have to come up with some wrapper structure to persist the error from the rejection or smth.

Maybe nodejs team could consider the direction taken by eslint/no-floating-promises of not treating promises assigned to a variable as unhandled?

If developer does not operate on the promise stored in the variable anyhow, the linter/IDE will warn him that it's unused, while when he will, he'll get the shutdown check again when he'll try to use this variable, so no unhandled promise coding mistakes should be missed still.

Of course I can always manually add an unhandledRejection event listener or use a command line flag, but that would seem like a workaround for an incorrectly written code, even though the code seems ok to me, and if it is indeed ok, that would mean people are forced to limit the way they code just because of some synthetic restriction, which does not seem nice.

[ExE-Boss]

@klesun
That is the behaviour of V8.

Node simply registers a callback to V8’s API for HostPromiseRejectionTracker, which causes the program to terminate if V8 determines that no rejection handler is being added to the promise.

---

To fix your case, you need to attach a no-op handler:

const whenSomeOptionalAsyncData = getSomeOptionalAsyncData();
// Attach a no-op rejection handler, so that the process doesn't exit
// if `whenSomeOptionalAsyncData` is a rejected promise:
whenSomeOptionalAsyncData.catch(() => {});
...
return {
	// Use async function to create a new, possibly rejected promise without
	// the above rejection handler, so that code which uses this API
	// is required to attach its own rejection handler:
	getSomeOptionalAsyncData: async () => whenSomeOptionalAsyncData,
};