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