Skip to content

lib/src: exit on gc unhandled promise #15126

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.

Sign up for GitHub

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

Closed
BridgeAR wants to merge 6 commits into from
Closed

lib/src: exit on gc unhandled promise #15126

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
711d699
lib,src: throw on unhanded promise rejections
Fishrock123 Apr 25, 2016
af11384
lib: follow up to exit early
BridgeAR Aug 31, 2017
19c7eb3
[squash] fix linter errors
BridgeAR Sep 1, 2017
69bfdd6
[squash] fix a test and remove unnecessary fn
BridgeAR Sep 1, 2017
5d2c83e
[squash] address comments
BridgeAR Sep 26, 2017
8fd484e
[squash] fix test
BridgeAR Sep 26, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
lib: follow up to exit early
  • Loading branch information
@BridgeAR
BridgeAR committed Sep 26, 2017
commit af11384393897286a30190d6621bb21718a115d0
9 changes: 0 additions & 9 deletions doc/api/deprecations.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,15 +190,6 @@ Type: End-of-Life
`Intl.v8BreakIterator` was a non-standard extension and has been removed.
See [`Intl.Segmenter`](https://github.com/tc39/proposal-intl-segmenter).

<a id="DEP0018"></a>
### DEP0018: Unhandled promise rejections

Type: Runtime

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.

@silverwind silverwind Sep 1, 2017

Copy link
Copy Markdown
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?

@targos targos Sep 1, 2017

Copy link
Copy Markdown
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

<a id="DEP0019"></a>
### DEP0019: require('.') resolved outside directory

Expand Down
67 changes: 37 additions & 30 deletions lib/internal/process/promises.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,13 @@

const { safeToString } = process.binding('util');

const promiseRejectEvent = process._promiseRejectEvent;
const hasBeenNotifiedProperty = new WeakMap();
const pendingUnhandledRejections = [];

exports.setup = setupPromises;

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

@benjamingr benjamingr Sep 1, 2017

Copy link
Copy Markdown
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?

@benjamingr benjamingr Sep 1, 2017

Copy link
Copy Markdown
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)

@Jessidhia Jessidhia Sep 20, 2017

Copy link
Copy Markdown

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.

@BridgeAR BridgeAR Sep 26, 2017

Copy link
Copy Markdown
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.

const promiseToGuidProperty = new Map();
const pendingUnhandledRejections = [];
const promiseInternals = {};
let lastPromiseId = 1;

process._setupPromises(function(event, promise, reason) {
if (event === promiseRejectEvent.unhandled)
Expand All @@ -20,32 +18,48 @@ function setupPromises(scheduleMicrotasks) {
else
require('assert').fail(null, null, 'unexpected PromiseRejectEvent');
}, function getPromiseReason(data) {
return data[data.indexOf('[[PromiseValue]]') + 1];
// TODO: Remove this function as it should not be necessary.
return data[3];
}, promiseInternals);

function unhandledRejection(promise, reason) {
hasBeenNotifiedProperty.set(promise, false);
promiseToGuidProperty.set(promise, lastPromiseId++);
addPendingUnhandledRejection(promise, reason);
}

function rejectionHandled(promise) {
const hasBeenNotified = hasBeenNotifiedProperty.get(promise);
if (hasBeenNotified !== undefined) {
hasBeenNotifiedProperty.delete(promise);
if (hasBeenNotified === true) {
promiseInternals.untrackPromise(promise);
process.nextTick(function() {
process.emit('rejectionHandled', promise);
});
hasBeenNotifiedProperty.delete(promise);
if (hasBeenNotified) {

@benjamingr benjamingr Sep 1, 2017

Copy link
Copy Markdown
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

@BridgeAR BridgeAR Sep 26, 2017

Copy link
Copy Markdown
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.

const uid = promiseToGuidProperty.get(promise);
promiseToGuidProperty.delete(promise);
let warning = null;
if (!process.listenerCount('rejectionHandled')) {
// Generate the warning object early to get a good stack trace.
warning = new Error('Promise rejection was handled ' +
`asynchronously (rejection id: ${uid})`);
}

promiseInternals.untrackPromise(promise);
process.nextTick(function() {
if (!process.emit('rejectionHandled', promise)) {
if (warning === null)
warning = new Error('Promise rejection was handled ' +
`asynchronously (rejection id: ${uid})`);
warning.name = 'PromiseRejectionHandledWarning';
warning.id = uid;
process.emitWarning(warning);
}
});
} else {
promiseToGuidProperty.delete(promise);
}
}

function emitWarning(uid, reason) {
const warning = new Error(
`Unhandled promise rejection (rejection id: ${uid}): ` +
safeToString(reason));
function emitWarning(promise, reason) {
const uid = promiseToGuidProperty.get(promise);
const warning = new Error('Unhandled promise rejection ' +
`(rejection id: ${uid}): ${safeToString(reason)}`);
warning.name = 'UnhandledPromiseRejectionWarning';
warning.id = uid;
try {
Expand All @@ -56,14 +70,6 @@ function setupPromises(scheduleMicrotasks) {
// ignored
}
process.emitWarning(warning);
if (!deprecationWarned) {
deprecationWarned = true;
process.emitWarning(
'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.',
'DeprecationWarning', 'DEP0018');
}
}

function emitPendingUnhandledRejections() {
Expand All @@ -75,6 +81,7 @@ function setupPromises(scheduleMicrotasks) {
hasBeenNotifiedProperty.set(promise, true);
if (!process.emit('unhandledRejection', reason, promise)) {
promiseInternals.trackPromise(promise);
emitWarning(promise, reason);
} else {
hadListeners = true;
}
Expand All @@ -89,4 +96,4 @@ function setupPromises(scheduleMicrotasks) {
}

return emitPendingUnhandledRejections;
}
};
35 changes: 2 additions & 33 deletions node.gyp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -657,39 +657,7 @@
'<(SHARED_INTERMEDIATE_DIR)', # for node_natives.h
],

'libraries': [
'<(OBJ_GEN_PATH)/node_javascript.<(OBJ_SUFFIX)',
'<(OBJ_PATH)/node_debug_options.<(OBJ_SUFFIX)',
'<(OBJ_PATH)/async-wrap.<(OBJ_SUFFIX)',
'<(OBJ_PATH)/env.<(OBJ_SUFFIX)',
'<(OBJ_PATH)/node.<(OBJ_SUFFIX)',
'<(OBJ_PATH)/node_buffer.<(OBJ_SUFFIX)',
'<(OBJ_PATH)/node_i18n.<(OBJ_SUFFIX)',
'<(OBJ_PATH)/node_url.<(OBJ_SUFFIX)',
'<(OBJ_PATH)/debug-agent.<(OBJ_SUFFIX)',
'<(OBJ_PATH)/util.<(OBJ_SUFFIX)',
'<(OBJ_PATH)/string_bytes.<(OBJ_SUFFIX)',
'<(OBJ_PATH)/string_search.<(OBJ_SUFFIX)',
'<(OBJ_PATH)/stream_base.<(OBJ_SUFFIX)',
'<(OBJ_PATH)/node_constants.<(OBJ_SUFFIX)',
'<(OBJ_PATH)/node_revert.<(OBJ_SUFFIX)',
'<(OBJ_TRACING_PATH)/agent.<(OBJ_SUFFIX)',
'<(OBJ_TRACING_PATH)/node_trace_buffer.<(OBJ_SUFFIX)',
'<(OBJ_TRACING_PATH)/node_trace_writer.<(OBJ_SUFFIX)',
'<(OBJ_TRACING_PATH)/trace_event.<(OBJ_SUFFIX)',
'<(OBJ_PATH)/track-promise.<(OBJ_SUFFIX)',
],

'defines': [
# gtest's ASSERT macros conflict with our own.
'GTEST_DONT_DEFINE_ASSERT_EQ=1',
'GTEST_DONT_DEFINE_ASSERT_GE=1',
'GTEST_DONT_DEFINE_ASSERT_GT=1',
'GTEST_DONT_DEFINE_ASSERT_LE=1',
'GTEST_DONT_DEFINE_ASSERT_LT=1',
'GTEST_DONT_DEFINE_ASSERT_NE=1',
'NODE_WANT_INTERNALS=1',
],
'defines': [ 'NODE_WANT_INTERNALS=1' ],

'sources': [
'src/node_platform.cc',
Expand Down Expand Up @@ -727,6 +695,7 @@
'<(OBJ_TRACING_PATH)<(OBJ_SEPARATOR)node_trace_buffer.<(OBJ_SUFFIX)',
'<(OBJ_TRACING_PATH)<(OBJ_SEPARATOR)node_trace_writer.<(OBJ_SUFFIX)',
'<(OBJ_TRACING_PATH)<(OBJ_SEPARATOR)trace_event.<(OBJ_SUFFIX)',
'<(OBJ_PATH)<(OBJ_SEPARATOR)track-promise.<(OBJ_SUFFIX)',
],
}],
['v8_enable_inspector==1', {
Expand Down
2 changes: 1 addition & 1 deletion src/env.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@
#include "handle_wrap.h"
#include "req-wrap.h"
#include "track-promise.h"
#include "tree.h"
#include "util.h"
#include "uv.h"
#include "v8.h"
Expand Down Expand Up @@ -316,6 +315,7 @@ class ModuleWrap;
V(performance_entry_callback, v8::Function) \
V(performance_entry_template, v8::Function) \
V(process_object, v8::Object) \
V(promise_wrap_template, v8::ObjectTemplate) \
V(promise_unhandled_rejection_function, v8::Function) \
V(promise_unhandled_rejection, v8::Function) \
V(push_values_to_array_function, v8::Function) \
Expand Down
8 changes: 1 addition & 7 deletions src/node.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1316,7 +1316,7 @@ Local<Value> GetPromiseReason(Environment* env, Local<Value> promise) {

// If fn is empty we'll almost certainly have to panic anyways
return fn->Call(env->context(), Null(env->isolate()), 1,
&internal_props).ToLocalChecked();
&internal_props).ToLocalChecked();
}


Expand Down Expand Up @@ -3786,12 +3786,6 @@ void LoadEnvironment(Environment* env) {
// Add a reference to the global object
Local<Object> global = env->context()->Global();

Local<Object> js_array_object = global->Get(
FIXED_ONE_BYTE_STRING(env->isolate(), "Array")).As<Object>();
Local<Function> js_array_from_function = js_array_object->Get(
FIXED_ONE_BYTE_STRING(env->isolate(), "from")).As<Function>();
env->set_array_from(js_array_from_function);

#if defined HAVE_DTRACE || defined HAVE_ETW
InitDTrace(env, global);
#endif
Expand Down
6 changes: 5 additions & 1 deletion test/message/promise_fast_handled_reject.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
'use strict';
const common = require('../common');
const assert = require('assert');

const p1 = new Promise((res, rej) => {
consol.log('One'); // eslint-disable-line no-undef
Expand All @@ -20,7 +21,10 @@ new Promise((res, rej) => {
}), 1);
});

process.on('unhandledRejection', () => {}); // Ignore
process.on('rejectionHandled', () => {}); // Ignore

@benjamingr benjamingr Sep 1, 2017

Copy link
Copy Markdown
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.

@BridgeAR BridgeAR Sep 21, 2017

Copy link
Copy Markdown
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.


process.on('uncaughtException', (err) =>
common.fail('Should not trigger uncaught exception'));
assert.fail('Should not trigger uncaught exception'));

process.on('exit', () => process._rawDebug('exit event emitted'));
4 changes: 2 additions & 2 deletions test/message/promise_fast_handled_reject.out
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,12 @@ exit event emitted

ReferenceError: consol is not defined
at *test*message*promise_fast_handled_reject.js:*:*
at Promise (<anonymous>)
at Object.<anonymous> (*test*message*promise_fast_handled_reject.js:*:*)
at Module._compile (module.js:*:*)
at Object.Module._extensions..js (module.js:*:*)
at Module.load (module.js:*:*)
at tryModuleLoad (module.js:*:*)
at Function.Module._load (module.js:*:*)
at Module.runMain (module.js:*:*)
at run (bootstrap_node.js:*:*)
at Function.Module.runMain (module.js:*:*)
at startup (bootstrap_node.js:*:*)
7 changes: 5 additions & 2 deletions test/message/promise_fast_reject.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@

// We should always have the stacktrace of the oldest rejection.

@benjamingr benjamingr Sep 1, 2017

Copy link
Copy Markdown
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 this is behavior we can rely on - if V8 decides to GC 'Two' first that's entirely in its right.

@BridgeAR BridgeAR Sep 21, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I somewhat agree. It seems like we can rely on it for now though. Would you still want this to be changed?


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

new Promise(function(res, rej) {
consol.log('One'); // eslint-disable-line no-undef
Expand All @@ -12,7 +13,9 @@ new Promise(function(res, rej) {
consol.log('Two'); // eslint-disable-line no-undef
});

process.on('unhandledRejection', () => {}); // Ignore

process.on('uncaughtException', (err) =>
common.fail('Should not trigger uncaught exception'));
assert.fail('Should not trigger uncaught exception'));

process.on('exit', () => process._rawDebug('exit event emitted'));
4 changes: 2 additions & 2 deletions test/message/promise_fast_reject.out
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,12 @@ exit event emitted

ReferenceError: consol is not defined
at *test*message*promise_fast_reject.js:*:*
at Promise (<anonymous>)
at Object.<anonymous> (*test*message*promise_fast_reject.js:*:*)
at Module._compile (module.js:*:*)
at Object.Module._extensions..js (module.js:*:*)
at Module.load (module.js:*:*)
at tryModuleLoad (module.js:*:*)
at Function.Module._load (module.js:*:*)
at Module.runMain (module.js:*:*)
at run (bootstrap_node.js:*:*)
at Function.Module.runMain (module.js:*:*)
at startup (bootstrap_node.js:*:*)
25 changes: 11 additions & 14 deletions test/message/promise_reject.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,27 +1,24 @@
// Flags: --expose-gc

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

// Flags: --expose-gc
const common = require('../common');
const assert = require('assert');

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

// Manually call GC due to possible memory contraints with attempting to
// Manually call GC due to possible memory constraints with attempting to

@benjamingr benjamingr Sep 1, 2017

Copy link
Copy Markdown
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)

@BridgeAR BridgeAR Sep 21, 2017 •
edited
Loading

Copy link
Copy Markdown
Member Author

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.

@Fishrock123 Fishrock123 Oct 7, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

probably was arbitrary

// trigger it "naturally".
setTimeout(common.mustCall(() => {
/* eslint-disable no-undef */
gc();
gc();
gc();
/* eslint-enable no-undef */
}, 1), 2);
global.gc();
global.gc();
global.gc();
}), 2);

process.on('beforeExit', () =>
common.fail('beforeExit should not be reached'));
process.on('unhandledRejection', () => {}); // Ignore

process.on('uncaughtException', (err) => {
// XXX(Fishrock123): This test is currently broken...
console.log(err.stack);
common.fail('Should not trigger uncaught exception');
assert.fail('Should not trigger uncaught exception');
});

process.on('exit', () => process._rawDebug('exit event emitted'));
3 changes: 1 addition & 2 deletions test/message/promise_reject.out
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ Error: oops
at Module.load (module.js:*:*)
at tryModuleLoad (module.js:*:*)
at Function.Module._load (module.js:*:*)
at Module.runMain (module.js:*:*)
at run (bootstrap_node.js:*:*)
at Function.Module.runMain (module.js:*:*)
at startup (bootstrap_node.js:*:*)
at bootstrap_node.js:*:*
12 changes: 1 addition & 11 deletions test/message/unhandled_promise_trace_warnings.out
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,18 +8,8 @@
at *
at *
at *
(node:*) [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.
at *
at *
at *
at *
at *
at *
at *
at *
(node:*) PromiseRejectionHandledWarning: Promise rejection was handled asynchronously (rejection id: 1)
at getAsynchronousRejectionWarningObject (internal/process/promises.js:*)
at rejectionHandled (internal/process/promises.js:*)
at *
at *
at Promise.then *
at Promise.catch *
Expand Down
10 changes: 3 additions & 7 deletions test/parallel/test-promises-gc-before-handled.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,26 +1,22 @@
// Flags: --expose-gc

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

// Flags: --expose-gc

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

// Manually call GC due to possible memory contraints with attempting to
// Manually call GC due to possible memory constraints with attempting to
// trigger it "naturally".
setTimeout(common.mustCall(() => {
/* eslint-disable no-undef */
gc();
gc();
gc();
/* eslint-enable no-undef */
setTimeout(common.mustCall(() => {
/* eslint-disable no-undef */
gc();
gc();
gc();
/* eslint-enable no-undef */
setTimeout(common.mustCall(() => {
p.catch(() => {});
}, 1), 250);
Expand Down
Loading