src: exit on gc unhandled rejection #20097

BridgeAR · 2018-04-17T12:24:28Z

This is a minimal implementation of exiting on garbage collected unhandled rejections.

As soon as a unhandled rejection is collected, it will trigger a fatal exception and exit with code 1 and print the error.
It will keep emitting warnings as it used to to and the user will be notified in case there are false negatives for the GC (a truly unhandled rejection is not garbage collected).

The warnings have quite some room for performance optimization but I wanted to keep this PR to the bare minimum so it is only about the functionality itself. I plan on working on optimizing the performance after this landed.

This work is based on former work from @Fishrock123 and @addaleax.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

src/node.cc

lib/internal/process/promises.js

src/node.cc

addaleax · 2018-04-17T15:28:36Z

lib/internal/bootstrap/node.js

@@ -426,23 +426,23 @@
      emitAfter
    } = NativeModule.require('internal/async_hooks');

-    process._fatalException = function(er) {
+    process._fatalException = function(er, fromPromise) {


The only point of fromPromise is skipping the uncaughtException handler – is that correct? If so, can you explain the motivation behind it?

(Also, if that is correct: Could we name it something like skipUncaughtExceptionListener or so, to be a bit more descriptive?)

That is correct. In this case I just used the original implementation. I personally have no strong opinion about having a uncaughtException in this case. In a way it would make sense to have that.

I'm +1 on calling uncaughtException handlers on unhandled rejections. I think it would be very surprising to not have them called - especially for APMs.

Note that this would also mean we'd have to deal with the case people chose to ignore the error rather than exit the process - like they can in an "unhandledRejection" handler.

I'm +1 to route unhandled rejection into 'uncaughtException' at this point.

@ljharb that would not be possible with the suggested implementation. What could be done is to wrap the error into another error that reports about being a unhandled rejection.

@benjamingr that would not be the case with the suggested change. It would work as I explained above. So having a unhandledRejection handler will not prevent the process to exit. That would completely depend on uncaughtException.

@BridgeAR I understand and I don't think that's correct semantics. I think it would surprise users - you'd expect to be able to intercept any unhandledRejection. It's very common to use it to throw and cause the uncaughtException today - or suppress termination in certain cases like:

process.on('unhandledRejection', err => { if (err.isImportant) log(err); if (err.isSpecificKind) return; // not an error, a bug with some library I'm using throw err; });

It is possible to intercept the unhandled rejections. But the error on GC is a different story.

By the way: without routing the unhandled rejection to uncaught exception, it would not be possible to stop the process from exiting.

It is a anti pattern to continue the program in case a exception is thrown. The pattern about ignoring specific errors is not something we should promote out of my perspective. In case a library is handling something wrong, the library should be fixed or not used.

To anyone reading - we went on to gather some real user data and the contents of this thread is invalid. See the proposed semantics in nodejs/promise-use-cases#27

src/node.cc

Fishrock123 · 2018-04-17T16:59:29Z

It appears to me that this has hole that original doesn't:

The information gets lots entirely if your process exists prematurely. Even if the promise remains unhandled to process end.

That's why I did iteration on process end to check if there were unhandled rejections lying around. GC on promises is so slow that it is only likely to trigger on long-running applications, even if silent error(s) occurred and state is undefined in shorter scripts.

Fishrock123 · 2018-04-17T17:00:06Z

I suppose that could be done separately, but it seems deeply connected to the end behavior to me.

BridgeAR · 2018-04-17T17:13:31Z

@Fishrock123 ~~I am not sure I can follow. In what way does the information get lost? Can you give me an example?~~ I see your point but I would rather get this in without that first. Adding something like that later on should just be semver-minor as far as I can tell, since it would not interfere with the regular program flow and just log stuff on exit.

BridgeAR · 2018-04-17T17:17:24Z

I addressed some comments.

src/node.cc

BridgeAR · 2018-04-17T23:03:47Z

Just a heads up: I am currently flooded and I am going to work on this in one or two days again.

mhdawson · 2018-07-17T22:27:32Z

@BridgeAR what's your thought on the suggestion that this be landed as experimental to start? It seems like it might make sense to me.

benjamingr · 2018-07-18T11:24:14Z

@mhdawson

@BridgeAR what's your thought on the suggestion that this be landed as experimental to start? It seems like it might make sense to me.

The current plan is to land this as opt-in to start with users having to explicitly define an environment variable or use a command-line flag. The idea is to change this behaviour to the default at a later point.

jasnell · 2018-08-08T15:40:54Z

doc/api/cli.md

+
+Setting this environment variable allows fine grained control over the behavior
+of unhandled rejections. This may be set to either one of `SILENT`, `WARN`,
+`ERROR_ON_GC`, `ERROR` or `STRICT` while the default behavior without using the


just a nit... make the values lower-case. They tend to be easier for users. --unhandled-rejections=error_on_gc

They will be accepted in all cases but it should be easier to read them being upper case.

It should have received a variable. Instead, it was set to a string that was coerced to a integer later on. This fixes it by correctly passing through the variable value.

This adds a fast path to the unhandledRejections and handledRejections to actually stop trying to emit something if no hook is attached.

It is not necessary to run the microtasks when an unhandledRejection hook is attached.

fixup: undo deprecation change

jasnell · 2018-10-16T18:31:11Z

Ping @BridgeAR ... given the discussion last week, do you plan on updating this PR or opening a new one with the agreed upon updates?

BridgeAR · 2018-10-16T18:45:38Z

I am fine either way. I would likely just go ahead and update the PR.

ljharb · 2018-10-16T18:57:21Z

Can someone summarize what updates were agreed upon?

devsnek · 2018-10-16T19:06:47Z

@ljharb we agreed to move forward on a silent mode, warning mode, and always exit mode. however four people (including myself) disagreed with the existence of gc mode, and no one in the room could think of any good reason why anyone would want to use a gc mode.

i don't recall though if we agreed on a default... maybe it happened after the zoom cut out.

targos · 2018-10-16T19:09:13Z

Add an opt-in flag to let the user choose between 3 ways for Node to react to unhandled rejections:

Silent: Node does nothing
Warn: Node emits a warning. This would be similar to the current behavior but with a plain warning instead of a deprecation warning
Throw: Node throws an uncaughtException

mmarchini · 2018-10-16T19:13:30Z

Actually, when @BridgeAR asked us who disagreed with Exit on GC there were more than 4 people with their hands up.

i don't recall though if we agreed on a default

We agreed that default is out of scope for this PR and should be discussed separately.

devsnek · 2018-10-16T19:20:24Z

@mmarchini thanks for the correction, i'm glad it was more :)

boneskull · 2018-12-21T08:04:15Z

Dear Santa,

I haven't written in awhile, sorry. I found myself at this PR after clicking through three previously-closed PRs all the way back to April 2016. Then I read through nodejs/promises#26, where it sounded like abort-on-GC was accepted as a fair solution.

A full two years later, when this PR was opened, it was still a fair solution.

Now, I'm reading Node.js need a flag to choose between three behaviors; one of which is the current behavior, another is seemingly the same as --no-warnings, and the other is... really confusing. I think there's a fourth, which is "abort on Promise" that some people seem to like.

This is comically frustrating. If collaborators can't even agree on which behavior(s) to provide, how could they pick a default?

So here's my xmas list. I realize this is a big ask.

Collaborators agree that Node's current behavior is distasteful and confusing for a wide range of Node.js users, especially those new to Promises.
Collaborators would understand that anything more complex than an "on/off" behavior would negatively impact module maintainers (and the users of those modules) who must add code to account for the behaviors.
Collaborators put aside personal preferences, opinions or habits, and instead refer to the previous two items on this list.
@BridgeAR or another generous soul would resume work on this PR.
Node.js' TSC:
1. Recognizes this has gone on for far too long, at the expense of Node.js' users.
2. Commits to a solution per their decision-making process.
3. Commits to a plan to introduce new default behavior after time behind an experimental flag.

your servant in darkness,
Chris

P.S. If I don't drop dead from embarrassment, you will find milk & cookies on the counter next to the whiskey and aspirin.

BridgeAR · 2019-03-11T20:35:17Z

I opened a alternative PR: #26599

BridgeAR requested review from apapirovski, mcollina, bnoordhuis, jasnell, addaleax, Fishrock123, TimothyGu, targos and a team April 17, 2018 12:24

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. process Issues and PRs related to the process subsystem. labels Apr 17, 2018

This comment has been minimized.

Sign in to view

BridgeAR added this to the 10.0.0 milestone Apr 17, 2018

BridgeAR added the semver-major PRs that contain breaking changes and should be released in the next major version. label Apr 17, 2018

This comment has been minimized.

Sign in to view

BridgeAR removed this from the 10.0.0 milestone Apr 17, 2018

BridgeAR mentioned this pull request Apr 17, 2018

lib/src: exit on gc unhandled promise #15126

Closed

4 tasks

mcollina reviewed Apr 17, 2018

View reviewed changes

src/node.cc Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

targos reviewed Apr 17, 2018

View reviewed changes

lib/internal/process/promises.js Outdated Show resolved Hide resolved

targos reviewed Apr 17, 2018

View reviewed changes

lib/internal/process/promises.js Outdated Show resolved Hide resolved

addaleax reviewed Apr 17, 2018

View reviewed changes

BridgeAR force-pushed the proper-unhandled-rejections branch from f9bcf1c to 333b0fb Compare April 17, 2018 17:11

addaleax reviewed Apr 17, 2018

View reviewed changes

src/node.cc Outdated Show resolved Hide resolved

devsnek mentioned this pull request Jul 17, 2018

util: expose promise events #21857

Closed

4 tasks

misterdjules mentioned this pull request Jul 18, 2018

Add Netflix survey nodejs/promises-debugging#12

Merged

misterdjules mentioned this pull request Jul 24, 2018

Follow up on July 3rd nodejs/promises-debugging#5

Open

jasnell reviewed Aug 8, 2018

View reviewed changes

BridgeAR mentioned this pull request Aug 9, 2018

provide CLI option to die on UnhandledPromiseRejectionWarning #22213

Closed

BridgeAR added 6 commits September 5, 2018 02:52

deps: fix V8_PROMISE_INTERNAL_FIELD_COUNT in features.gypi

6ed1985

It should have received a variable. Instead, it was set to a string that was coerced to a integer later on. This fixes it by correctly passing through the variable value.

src: exit on gc unhandled rejection

1eef8cf

process: stop rejection loop early

46b0c97

This adds a fast path to the unhandledRejections and handledRejections to actually stop trying to emit something if no hook is attached.

process: do not run extra microtasks

5218bb2

It is not necessary to run the microtasks when an unhandledRejection hook is attached.

fixup: further doc improvements and add todo

2dfe16a

fixup: undo deprecation change

fixup: address targos comments

637824f

BridgeAR force-pushed the proper-unhandled-rejections branch from 77b2bcf to 637824f Compare September 5, 2018 10:15

misterdjules mentioned this pull request Sep 25, 2018

[Discuss] Resolving the unhandled rejections issue #22822

Closed

BridgeAR mentioned this pull request Oct 9, 2018

Summit Topic: Promises openjs-foundation/summit#119

Closed

BridgeAR mentioned this pull request Mar 11, 2019

process: add --unhandled-rejections flag #26599

Closed

4 tasks

BridgeAR closed this Mar 11, 2019

devsnek mentioned this pull request Jan 1, 2020

Proposal for more correct detection and reporting of unhandled promise #31148

Closed

BridgeAR deleted the proper-unhandled-rejections branch January 20, 2020 11:37

mmarchini mentioned this pull request Sep 15, 2020

Voting on the default behavior for Promise unhandled rejections nodejs/TSC#916

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: exit on gc unhandled rejection #20097

src: exit on gc unhandled rejection #20097

BridgeAR commented Apr 17, 2018 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

addaleax Apr 17, 2018

BridgeAR Apr 17, 2018

benjamingr Apr 18, 2018

benjamingr Apr 18, 2018

mcollina Apr 18, 2018

BridgeAR Apr 25, 2018

benjamingr Apr 25, 2018

BridgeAR Apr 25, 2018

This comment was marked as outdated.

benjamingr Jun 11, 2018

Fishrock123 commented Apr 17, 2018

Fishrock123 commented Apr 17, 2018

BridgeAR commented Apr 17, 2018 •

edited

Loading

BridgeAR commented Apr 17, 2018

BridgeAR commented Apr 17, 2018

mhdawson commented Jul 17, 2018

benjamingr commented Jul 18, 2018

jasnell Aug 8, 2018

BridgeAR Sep 5, 2018

jasnell commented Oct 16, 2018

BridgeAR commented Oct 16, 2018

ljharb commented Oct 16, 2018

devsnek commented Oct 16, 2018 •

edited

Loading

targos commented Oct 16, 2018

mmarchini commented Oct 16, 2018

devsnek commented Oct 16, 2018

boneskull commented Dec 21, 2018

BridgeAR commented Mar 11, 2019

src: exit on gc unhandled rejection #20097

src: exit on gc unhandled rejection #20097

Conversation

BridgeAR commented Apr 17, 2018 • edited Loading

Checklist

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as outdated.

Choose a reason for hiding this comment

Fishrock123 commented Apr 17, 2018

Fishrock123 commented Apr 17, 2018

BridgeAR commented Apr 17, 2018 • edited Loading

BridgeAR commented Apr 17, 2018

BridgeAR commented Apr 17, 2018

mhdawson commented Jul 17, 2018

benjamingr commented Jul 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented Oct 16, 2018

BridgeAR commented Oct 16, 2018

ljharb commented Oct 16, 2018

devsnek commented Oct 16, 2018 • edited Loading

targos commented Oct 16, 2018

mmarchini commented Oct 16, 2018

devsnek commented Oct 16, 2018

boneskull commented Dec 21, 2018

BridgeAR commented Mar 11, 2019

BridgeAR commented Apr 17, 2018 •

edited

Loading

BridgeAR commented Apr 17, 2018 •

edited

Loading

devsnek commented Oct 16, 2018 •

edited

Loading