Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lib/src: exit on gc unhandled promise #15126

Closed
wants to merge 6 commits into from

Conversation

BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Sep 1, 2017

This is a follow up on #12010. Is is not complete and mainly a rebase with a few minor fixes.
All contributions go to @Fishrock123, @addaleax, @matthewloring and @ofrobots.

I have no idea about C++ and it is difficult for me to continue any further, therefore I would ask someone to step foward to continue the work here 😄

One N-API test is failing and I do not know how to properly fix that one.

Edit: I fixed the test but it seems like I have a regression somewhere in the error message printed. I am looking into that soon.

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
Affected core subsystem(s)

lib, src, doc

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Sep 1, 2017
@BridgeAR BridgeAR added the promises Issues and PRs related to ECMAScript promises. label Sep 1, 2017
@vkurchatkin vkurchatkin added the semver-major PRs that contain breaking changes and should be released in the next major version. label Sep 1, 2017
@addaleax
Copy link
Member

addaleax commented Sep 1, 2017

One N-API test is failing and I do not know how to properly fix that one.

I think explicitly silencing by adding a no-op unhandled rejection handler should be fine :)

@addaleax
Copy link
Member

addaleax commented Sep 1, 2017

By the way, the idea of adding a flag to Node that would restore the current behaviour was floated somewhere, and since then I have come to think that would be a really good thing to have.

@jasnell
Copy link
Member

jasnell commented Sep 1, 2017

Yeah. I've been working on this a bit this week in between other things. The strategy I've been working on is to use flags to enable a couple of possible behaviors with the current behavior set as the default. My goal has been to have a pull request sometime next week.

@BridgeAR
Copy link
Member Author

BridgeAR commented Sep 1, 2017

By the way, the idea of adding a flag to Node that would restore the current behaviour was floated somewhere, and since then I have come to think that would be a really good thing to have.

Is that not somewhat independent from this PR?

@addaleax
Copy link
Member

addaleax commented Sep 1, 2017

@BridgeAR Yes, but landing this PR and then introducing the flag would create a period where the current behaviour would not be available, and with respect to timing I’m a bit worried that might hit the Node 9 timeframe perfectly :| That’s all, I’d just like to make sure it’s at least considered.

@@ -1,3 +1,6 @@
(node:30929) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 1): ReferenceError: consol is not defined
(node:30929) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 2): ReferenceError: consol is not defined
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PID should be replaced with an asterisk for wildcard matching.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't it be marked EOL instead of being removed?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. It should be marked End-of-life

Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, thanks a lot for picking this up!

Left some comments, just to make sure we agree on the semantics we eventually want:

  • Not handling a promise within the "after-nextTick" should still produce a warning but not a deprecation warning. (TODO(Benjamingr) re-read our research from last time on the impact on async/await patterns)
  • Not handling a promise that performed GC should exit the process and print its stack trace like a regular exception.

const common = require('../common');

const p = new Promise((res, rej) => {
consol.log('oops'); // eslint-disable-line no-undef
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No reason to cause an implicit reference error here - this can just be throw new ReferenceError(...)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not want to change to much on the code and left the tests pretty much as they were. I will change it though.

const common = require('../common');

const p = new Promise((res, rej) => {
consol.log('oops'); // eslint-disable-line no-undef
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No reason to cause an implicit reference error here - this can just be throw new ReferenceError(...)

// Manually call GC due to possible memory constraints with attempting to
// trigger it "naturally".
setTimeout(common.mustCall(() => {
global.gc();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks really flakey, why 3 times? Why these delays in the setTimeout?

Are we testing that V8 doesn't drop a live reference in GC here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have been forced to check gc-based behaviour in setTimeouts in other cases in recent V8 versions. I don’t think anybody of has really gotten to the ground of it so far.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I see, then a comment would be nice.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not certain how that comment should look like. Do you have a example?


Promise.reject(new Error('oops'));

// Manually call GC due to possible memory constraints with attempting to
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this inside a setTimeout with 2 delay? Can't the handlers be added prior and then the global.gc calls made synchronously? (Also, why 3 times)

Copy link
Member Author

@BridgeAR BridgeAR Sep 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above for the 3 times. I did not write these tests originally and it is somewhat difficult for me to know why it was exactly written the way it is. I think the two milliseconds delay is arbitrary. I am also not sure about your second question. As far as I see it the setTimeout is meant to make sure the rejected promise is fully recognized to be unhandled.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably was arbitrary

const assert = require('assert');

new Promise(function(res, rej) {
consol.log('One'); // eslint-disable-line no-undef
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto about explicit errors and throwing directly rather than consol

consol.log('Three'); // eslint-disable-line no-undef
});

new Promise((res, rej) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what this tests, new Promise runs synchronously

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. It should probably be a nextTick instead.

}), 1);
});

process.on('rejectionHandled', () => {}); // Ignore
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default behavior for rejectionHandled is ignore.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will print a warning otherwise and I wanted to have the output as clean as possible.

@@ -6,6 +6,8 @@ const assert = require('assert');

let expected_result, promise;

process.on('unhandledRejection', () => {});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we fix the test instead of ignoring the errors?

If we can't it might indicate a design problem

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@addaleax recommended to ignore this. I did not find the cause of the issue here especially as the output is really little.

let deprecationWarned = false;
exports.setup = function setup(scheduleMicrotasks) {
const promiseRejectEvent = process._promiseRejectEvent;
const hasBeenNotifiedProperty = new Map();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain why a regular Map is sufficient here?

A WeakMap was used for a very specific reason in order to not interfere with GC.

On a related note, is there any way we can test the unhandled rejection tracking code does not leak memory?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I realize it doesn't do the same thing exactly and tracking happens in C++, but still)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WeakMaps can't be enumerated, but no enumeration is done here; when rejectionHandled is called there is still a strong reference to the promise (the promise argument) so it should still exist in the WeakMap 🤔

Not sure why a Map is needed either; it just creates the need for .delete calls.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason why I changed it is that I can not see a reason to use WeakMap. All entries are either going to be handled and in that case explicitly deleted or they result in a unhandled rejection that will now terminate the process. Using the WeakMap has a performance overhead and the GC has to do more work.

if (hasBeenNotified !== undefined) {
hasBeenNotifiedProperty.delete(promise);
hasBeenNotifiedProperty.delete(promise);
if (hasBeenNotified) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the .delete can be inside the if I think

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should always be deleted, otherwise the reference would be kept. This function is only called once per handled rejection and it can not be freed otherwise.

@benjamingr
Copy link
Member

By the way, the idea of adding a flag to Node that would restore the current behaviour was floated somewhere, and since then I have come to think that would be a really good thing to have.

The current behavior can be restored by adding an unhandledRejection handler and logging - if I understand correctly.

@bnoordhuis
Copy link
Member

I only looked at this briefly but it has the same issue as the old PR, doesn't it? It adds a lot of overhead per promise. Weakly persistent handles aren't cheap.

@benjamingr
Copy link
Member

@bnoordhuis yes, this isn't here yet - both behavior and overhead

@BridgeAR BridgeAR added the wip Issues and PRs that are still a work in progress. label Sep 5, 2017
@BridgeAR
Copy link
Member Author

BridgeAR commented Sep 26, 2017

I addressed the comments and rebased due to conflicts.

@BridgeAR
Copy link
Member Author

BridgeAR commented Oct 1, 2017

Ping

@mhdawson
Copy link
Member

mhdawson commented Oct 2, 2017

@BridgeAR not 100% sure by reading through the comments so far. Is this now in a state where its ready to land ? Just asking because of the earlier comments about not being there in terms of behavior and overhead.

Copy link
Contributor

@Fishrock123 Fishrock123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just pointing out an obvious bit of my WIP

there may be other spots too


// Make some sort of list size check so as to not leak memory.
if (env->promise_tracker_.Size() > 10000) {
// XXX(Fishrock123): Do some intelligent logic here?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for hopefully obvious reasons this shouldn't be merged in this state

Local<Value> err = GetPromiseReason(&env, orp);
Local<Message> message = Exception::CreateMessage(isolate, err);

// XXX(Fishrock123): Should this just call ReportException and
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

example 2

@mcollina
Copy link
Member

mcollina commented Oct 9, 2017

I am in favour of the approach. I would love to see this landed in Node 9.

@benjamingr
Copy link
Member

@BridgeAR status?

@BridgeAR BridgeAR added the notable-change PRs with changes that should be highlighted in changelogs. label Dec 15, 2017
@billinghamj
Copy link

billinghamj commented Jan 11, 2018

It looks like once this is merged, all code currently causing PromiseRejectionHandledWarning console messages to appear will immediately break. It sounds like a decision was reached some time ago.

Where can I go to find the discussion in which this decision was reached, so I can learn more about the reasoning etc.?

I think one could argue that asynchronously handling exceptions is not an invalid use of Promises. I also think you may be surprised by how many codebases will be affected - unless telemetry data is available, of course.

Edit: apologies, I now see that this will only occur when the object is GC'd - which will mean catching the Promise is impossible anyway.

Might be worth linking some more context/background in the PR description.

@targos
Copy link
Member

targos commented Feb 3, 2018

@BridgeAR do you still want to pursue this? I would like to see it land early enough for Node 10.

@BridgeAR
Copy link
Member Author

BridgeAR commented Feb 3, 2018

In general: yes. I am not sure if I can work on it next week though. The week after is probably possible.

But it is also fine for me if someone else wants to work on it right now.

@BridgeAR
Copy link
Member Author

Just a heads up: I am currently looking into this again and try to have a update about mid next week.

@BridgeAR
Copy link
Member Author

Closing in favor of #20097

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. notable-change PRs with changes that should be highlighted in changelogs. promises Issues and PRs related to ECMAScript promises. semver-major PRs that contain breaking changes and should be released in the next major version. wip Issues and PRs that are still a work in progress.
Projects
None yet
Development

Successfully merging this pull request may close these issues.