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.

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

Jump to bottom

lib/src: exit on gc unhandled promise #15126

Closed
wants to merge 6 commits into from
Closed

lib/src: exit on gc unhandled promise #15126

Show file tree
Hide file tree
Changes from all commits
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
8 changes: 4 additions & 4 deletions doc/api/deprecations.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,11 +193,11 @@ See [`Intl.Segmenter`](https://github.com/tc39/proposal-intl-segmenter).
<a id="DEP0018"></a>
### DEP0018: Unhandled promise rejections

Type: Runtime
Type: End-of-Life

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.
Any unhandled promise rejection that is garbage collected is going to exit the
process similar to unhandled exceptions. Please make sure to always handle all
possible rejections.

<a id="DEP0019"></a>
### DEP0019: require('.') resolved outside directory
Expand Down
4 changes: 2 additions & 2 deletions lib/internal/bootstrap_node.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -359,7 +359,7 @@
const { clearIdStack, asyncIdStackSize } = async_wrap;
const { kAfter, kCurrentAsyncId, kInitTriggerId } = async_wrap.constants;

process._fatalException = function(er) {
process._fatalException = function(er, fromPromise) {
var caught;

// It's possible that kInitTriggerId was set for a constructor call that
Expand All @@ -369,7 +369,7 @@
if (process.domain && process.domain._errorHandler)
caught = process.domain._errorHandler(er);

if (!caught)
if (!caught && !fromPromise)
caught = process.emit('uncaughtException', er);

// If someone handled it, then great. otherwise, die in C++ land
Expand Down
84 changes: 35 additions & 49 deletions lib/internal/process/promises.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,21 +2,13 @@

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

const promiseRejectEvent = process._promiseRejectEvent;
const hasBeenNotifiedProperty = new WeakMap();
const promiseToGuidProperty = new WeakMap();
const pendingUnhandledRejections = [];
let lastPromiseId = 1;

exports.setup = setupPromises;

function getAsynchronousRejectionWarningObject(uid) {
return new Error('Promise rejection was handled ' +
`asynchronously (rejection id: ${uid})`);
}

function setupPromises(scheduleMicrotasks) {
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)

Copy link

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.

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

process._setupPromises(function(event, promise, reason) {
if (event === promiseRejectEvent.unhandled)
Expand All @@ -25,7 +17,7 @@ function setupPromises(scheduleMicrotasks) {
rejectionHandled(promise);
else
require('assert').fail(null, null, 'unexpected PromiseRejectEvent');
});
}, promiseInternals);

function unhandledRejection(promise, reason) {
hasBeenNotifiedProperty.set(promise, false);
Expand All @@ -35,34 +27,36 @@ function setupPromises(scheduleMicrotasks) {

function rejectionHandled(promise) {
const hasBeenNotified = hasBeenNotifiedProperty.get(promise);
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.

const uid = promiseToGuidProperty.get(promise);
promiseToGuidProperty.delete(promise);
if (hasBeenNotified === true) {
let warning = null;
if (!process.listenerCount('rejectionHandled')) {
// Generate the warning object early to get a good stack trace.
warning = getAsynchronousRejectionWarningObject(uid);
}
process.nextTick(function() {
if (!process.emit('rejectionHandled', promise)) {
if (warning === null)
warning = getAsynchronousRejectionWarningObject(uid);
warning.name = 'PromiseRejectionHandledWarning';
warning.id = uid;
process.emitWarning(warning);
}
});
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 @@ -73,14 +67,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 @@ -90,9 +76,9 @@ function setupPromises(scheduleMicrotasks) {
const reason = pendingUnhandledRejections.shift();
if (hasBeenNotifiedProperty.get(promise) === false) {
hasBeenNotifiedProperty.set(promise, true);
const uid = promiseToGuidProperty.get(promise);
if (!process.emit('unhandledRejection', reason, promise)) {
emitWarning(uid, reason);
promiseInternals.trackPromise(promise);
emitWarning(promise, reason);
} else {
hadListeners = true;
}
Expand All @@ -107,4 +93,4 @@ function setupPromises(scheduleMicrotasks) {
}

return emitPendingUnhandledRejections;
}
};
4 changes: 4 additions & 0 deletions node.gyp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,6 +228,7 @@
'src/stream_wrap.cc',
'src/tcp_wrap.cc',
'src/timer_wrap.cc',
'src/track-promise.cc',
'src/tracing/agent.cc',
'src/tracing/node_trace_buffer.cc',
'src/tracing/node_trace_writer.cc',
Expand Down Expand Up @@ -270,6 +271,8 @@
'src/node_revert.h',
'src/node_i18n.h',
'src/pipe_wrap.h',
'src/track-promise.h',
'src/track-promise-inl.h',
'src/tty_wrap.h',
'src/tcp_wrap.h',
'src/udp_wrap.h',
Expand Down Expand Up @@ -692,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
3 changes: 2 additions & 1 deletion src/env-inl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,7 +280,8 @@ inline Environment* Environment::GetCurrent(

inline Environment::Environment(IsolateData* isolate_data,
v8::Local<v8::Context> context)
: isolate_(context->GetIsolate()),
: promise_tracker_(this),
isolate_(context->GetIsolate()),
isolate_data_(isolate_data),
async_hooks_(context->GetIsolate()),
timer_base_(uv_now(isolate_data->event_loop())),
Expand Down
9 changes: 8 additions & 1 deletion src/env.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
#endif
#include "handle_wrap.h"
#include "req-wrap.h"
#include "track-promise.h"
#include "util.h"
#include "uv.h"
#include "v8.h"
Expand Down Expand Up @@ -236,6 +237,7 @@ class ModuleWrap;
V(preference_string, "preference") \
V(priority_string, "priority") \
V(produce_cached_data_string, "produceCachedData") \
V(promise_rejection_index_string, "_promiseRejectionIndex") \
V(raw_string, "raw") \
V(read_host_object_string, "_readHostObject") \
V(readable_string, "readable") \
Expand Down Expand Up @@ -294,6 +296,7 @@ class ModuleWrap;
V(zero_return_string, "ZERO_RETURN")

#define ENVIRONMENT_STRONG_PERSISTENT_PROPERTIES(V) \
V(array_from, v8::Function) \
V(as_external, v8::External) \
V(async_hooks_destroy_function, v8::Function) \
V(async_hooks_init_function, v8::Function) \
Expand All @@ -312,8 +315,9 @@ class ModuleWrap;
V(performance_entry_callback, v8::Function) \
V(performance_entry_template, v8::Function) \
V(process_object, v8::Object) \
V(promise_reject_function, v8::Function) \
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) \
V(randombytes_constructor_template, v8::ObjectTemplate) \
V(script_context_constructor_template, v8::FunctionTemplate) \
Expand Down Expand Up @@ -678,6 +682,9 @@ class Environment {
bool RemovePromiseHook(promise_hook_func fn, void* arg);
bool EmitNapiWarning();

PromiseTracker promise_tracker_;
int64_t promise_tracker_index_ = 0;

private:
inline void ThrowError(v8::Local<v8::Value> (*fun)(v8::Local<v8::String>),
const char* errmsg);
Expand Down
Loading